Run unit tests in parallel #12926

pull sipa wants to merge 4 commits into bitcoin:master from sipa:201804_parunit changing 3 files +20 −13
  1. sipa commented at 8:35 pm on April 9, 2018: member

    This runs the unit tests (src/test/test_bitcoin) in 4 separate simultaneous processes, significantly speeding up some Travis runs (over 2x for win32).

    This uses an approach by @theuni that relies on make as the mechanism for distributing tests over processes (through -j). For every test .cpp file, we search for BOOST_FIXTURE_TEST_SUITE or BOOST_AUTO_TEST_SUITE, and then invoke the test binary for just that suite (using -t). The (verbose) output is stored in a temporary file, and only shown in the case of failure.

    Some makefile reshuffling is necessary to avoid trying to run tests from src/test/test_bitcoin.cpp for example, which contains framework/utility code but no real tests.

    Finally, order the Travis jobs from slow to fast (apart from the arm/doc job which goes first, for fast failure). This should help reducing the total wall clock time before opening a PR and finishing Travis, in case where not all jobs are started simultaneously.

    This is an alternative to #12831.

  2. in test/util/bitcoin-unit-tests.sh:6 in 242fe7c19a outdated 
    0@@ -0,0 +1,6 @@
1+#!/bin/bash
2+
3+PROCESSES=4
4+DIR="$(dirname "$0")"
5+
6+cat $DIR/../../src/{,wallet/,qt/}test/*.cpp | fgrep BOOST_FIXTURE_TEST_SUITE | cut -d '(' -f 2 | cut -d ',' -f 1 | shuf | xargs -n 1 -P $PROCESSES -I "{}" -- bash "$DIR/bitcoin-unit-test.sh" "{}/*"
    practicalswift commented at 8:51 pm on April 9, 2018:

    Nit: Double quote to prevent globbing and word splitting :-)

    cat "$DIR"/../../src/{,wallet/,qt/}test/*.cpp

    practicalswift commented at 9:08 pm on April 9, 2018:
    shuf is not available under OS X by default. Is sort -R sufficiently portable?
    sipa commented at 9:27 pm on April 9, 2018:
    Fixed (the files are now passed in from Makefile, as suggested by Cory).
    sipa commented at 9:27 pm on April 9, 2018:
    Fixed.
  3. in test/util/bitcoin-unit-tests.sh:3 in 242fe7c19a outdated 
    0@@ -0,0 +1,6 @@
1+#!/bin/bash
2+
3+PROCESSES=4
    jamesob commented at 8:54 pm on April 9, 2018:
    Would it make sense just to do xargs ... -P 0?
    sipa commented at 9:27 pm on April 9, 2018:
    I don’t like hardcoding the parallellism here, but with -P 0 my system will run all tests simultaneously - that’s a bit scary w.r.t. resource usage.
  4. in src/Makefile.test.include:153 in 242fe7c19a outdated 
    149@@ -151,6 +150,8 @@ bitcoin_test_clean : FORCE
150 	rm -f $(CLEAN_BITCOIN_TEST) $(test_test_bitcoin_OBJECTS) $(TEST_BINARY)
151 
152 check-local:
153+	@echo "Running test/util/bitcoin-unit-tests.py..."
    practicalswift commented at 9:02 pm on April 9, 2018:
    s/.py/.sh/ :-)
    sipa commented at 9:27 pm on April 9, 2018:
    Fixed.
  5. sipa force-pushed on Apr 9, 2018
  6. practicalswift commented at 9:31 pm on April 9, 2018: contributor

    Strong concept ACK.

    I like the straightforwardness and compactness of this solution vs. the previous attempt at parallelism.

    Do you have any speedup measurements?

  7. in src/Makefile.test.include:157 in e764ddcab7 outdated 
    149@@ -151,6 +150,8 @@ bitcoin_test_clean : FORCE
150 	rm -f $(CLEAN_BITCOIN_TEST) $(test_test_bitcoin_OBJECTS) $(TEST_BINARY)
151 
152 check-local:
153+	@echo "Running unit tests..."
154+	@$(top_builddir)/test/util/bitcoin-unit-tests.sh $(BITCOIN_TESTS)
155 	@echo "Running test/util/bitcoin-util-test.py..."
    practicalswift commented at 9:33 pm on April 9, 2018:
    Intentionally keeping bitcoin-util-test.py in there?
    sipa commented at 9:37 pm on April 9, 2018:
    These are unrelated tests.
    practicalswift commented at 9:44 pm on April 9, 2018:
    Oh, ofc – my bad. Sorry :-)
  8. sipa commented at 9:37 pm on April 9, 2018: member
    @practicalswift 48 minutes (when lucky) to 23 minutes for the win32 test in Travis.
  9. MarcoFalke commented at 9:43 pm on April 9, 2018: member

    48 minutes (when lucky) to 23 minutes for the win32 test in Travis.

    :tada:

  10. MarcoFalke commented at 9:45 pm on April 9, 2018: member
    utACK e764ddcab74755274856cf217cdeb83e2d5d9bc0
  11. practicalswift commented at 9:46 pm on April 9, 2018: contributor
    @sipa Wow! That will help us get rid of those nasty win32 Travis timeouts! :-)
  12. practicalswift commented at 9:49 pm on April 9, 2018: contributor

    utACK e764ddcab74755274856cf217cdeb83e2d5d9bc0

    My shell nits on this PR would have been communicated automatically (and hopefully fixed pre human review) if shellcheck linting was enabled in Travis. Please help review #12871 if you find shellcheck valuable :-)

  13. theuni commented at 10:04 pm on April 9, 2018: member

    48 minutes (when lucky) to 23 minutes for the win32 test in Travis.

    Nice! Reviewing.

  14. theuni commented at 10:07 pm on April 9, 2018: member 
    0cory@Macbook:~/dev/bitcoin/src$ sort -R
1sort: invalid option -- R

    :(

  15. practicalswift commented at 10:09 pm on April 9, 2018: contributor

    @theuni Oh, that’s bad! What version of sort are you using?

    FWIW:

    0$ sort --version
12.3-Apple (99)
2$ (echo A; echo B) | sort -R
3B
4A
5$ (echo A; echo B) | sort -R
6A
7B
  16. sipa commented at 10:11 pm on April 9, 2018: member
    If the random sorting is an issue we can just sort alphabetically. This would also let us do things like indicate which tests are big in their name for example, to put them earlier in the list.
  17. theuni commented at 10:15 pm on April 9, 2018: member

    @sipa The profile dir for tests is defined as:

    0pathTemp = fs::temp_directory_path() / strprintf("test_bitcoin_%lu_%i", (unsigned long)GetTime(), (int)(InsecureRandRange(100000)));

    As GetTime() has only second-level resolution, we may want to bump up the rand range a bit.

  18. practicalswift commented at 10:17 pm on April 9, 2018: contributor

    @sipa What about using awk?

    0$ (echo A; echo B) | awk 'BEGIN { srand(); } { printf("%.10f %s\n", rand(), $0); }' | \
1      sort | cut -f2- -d' '
2A
3B
4$ (echo A; echo B) | awk 'BEGIN { srand(); } { printf("%.10f %s\n", rand(), $0); }' | \
5      sort | cut -f2- -d' '
6B
7A
  19. sipa force-pushed on Apr 9, 2018
  20. sipa commented at 10:25 pm on April 9, 2018: member
    Removed random sorting (made it alphabetical), reordered the Travis tests from slow to fast (except the doc test which goes still first for fast-fail), and increased the test temp directory entropy.
  21. fanquake added the label Tests on Apr 9, 2018
  22. practicalswift commented at 10:27 pm on April 9, 2018: contributor
    utACK aa9be23327db7a12760f250e7261b5f6be6f9de0
  23. sipa commented at 10:28 pm on April 9, 2018: member

    Why the reordering of Travis builds? I’m not opposing it – just curious :-)

    In order to maximally exploit parallelism (of Travis jobs) you generally want to start the longest-running ones to start first. Sometimes not all jobs are started simultaneously (based on other load), in which case this matters in reducing the overall wall clock time spent between pushing and getting a final Travis result.

  24. tests: split up actual tests and helper files 66f32551bd
  25. tests: run tests in parallel 156db42c3f
  26. Reorder travis builds f6dfb0f504
  27. Increase entropy in test temp directory name 7ef9cd8491
  28. sipa force-pushed on Apr 10, 2018
  29. theuni commented at 1:08 am on April 10, 2018: member

    Reviewers: Sorry for the interruption, see the new approach here.

    I was discussing this earlier with @sipa and wanted to see if we could get this to work while preserving the make jobs. It turned out to be substantially simpler than what was in the PR already, and I guess @sipa doesn’t hate it too much :)

  30. sipa commented at 1:24 am on April 10, 2018: member
    Totally changed the approach, integrating everything into makefiles rather than creating external runner scripts (thanks @theuni). See updated PR description.
  31. jamesob commented at 2:17 am on April 10, 2018: member

    Tested ACK https://github.com/bitcoin/bitcoin/pull/12926/commits/7ef9cd8491185af26295b3501fca9d2a49379364

    Seeing a mild improvement locally :tada:

    201804_parunit

    make -j 4 check 282.46s user 14.91s system 252% cpu 1:57.88 total

    master

    make -j 4 check 193.71s user 10.10s system 129% cpu 2:37.31 total

  32. laanwj commented at 7:14 am on April 10, 2018: member

    Totally changed the approach, integrating everything into makefiles rather than creating external runner scripts (thanks @theuni). See updated PR description.

    Heh you implemented my IRC joke. I’m surprised that this works out to an elegant approach. But it’s really neat - will test.

    After already having built all, including the test executables, and commenting out secp256k1 and univalue tests (which take up significant time and aren’t relevant here):

    0user@ishtar:~/src/bitcoin/build$ time make -j10 check
1real    1m6.551s
2user    1m8.388s
3sys     0m2.392s
4
5(12926)user@ishtar:~/src/bitcoin/build$ time make -j10 check
6real    0m23.140s
7user    1m4.544s
8sys     0m8.616s

    Almost three times speed-up (on an 8-core machine). Nice!

  33. practicalswift commented at 9:23 am on April 10, 2018: contributor

    The new version is even better!

    Nice to avoid having two settings for concurrency (make -j and shell script variable).

  34. kallewoof commented at 9:39 am on April 10, 2018: member

    Seeing about 2x improvement. Tested ACK

    0master:         make -j10 check   97.74s user 52.14s system 138% cpu 1:48.19  total
1201804_parunit: make -j10 check  109.30s user 16.73s system 219% cpu   57.482 total
  35. jonasschnelli commented at 12:11 pm on April 10, 2018: contributor

    Tested ACK 7ef9cd8491185af26295b3501fca9d2a49379364

    OSX 10.13.4 (2.9 GHz Intel Core i7): time make -j5 V=1 check

    This PR:

    0real	1m28.438s
1user	3m42.053s
2sys	0m45.473s

    versus master:

    0real	3m5.687s
1user	2m57.911s
2sys	0m46.390s
  36. laanwj merged this on Apr 10, 2018
  37. laanwj closed this on Apr 10, 2018
  38. laanwj referenced this in commit dd1ca9e0b3 on Apr 10, 2018
  39. scravy referenced this in commit db2dbc9090 on Apr 1, 2019
  40. scravy referenced this in commit ccdeb51be5 on Apr 1, 2019
  41. PastaPastaPasta referenced this in commit e8962ff953 on Apr 13, 2021
  42. xdustinface referenced this in commit 2b145943db on Apr 14, 2021
  43. PastaPastaPasta referenced this in commit b929078ec8 on Apr 14, 2021
  44. PastaPastaPasta referenced this in commit 7f5e7885fb on Apr 15, 2021
  45. PastaPastaPasta referenced this in commit 460c1ee359 on Apr 17, 2021
  46. kittywhiskers referenced this in commit 53254c5dbf on Apr 23, 2021
  47. MarcoFalke locked this on Sep 8, 2021


sipa practicalswift jamesob MarcoFalke theuni laanwj kallewoof jonasschnelli

Labels
Tests

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-04 22:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me