build: speed up by flattening the dependency graph #30911

pull theuni wants to merge 2 commits into bitcoin:master from theuni:cmake-flatten-dependencies changing 4 files +19 −0
  1. theuni commented at 6:44 pm on September 16, 2024: member

    These changes speed up my build (default config/options/targets) by roughly 10%. I suspect the difference may be more significant in other build configs.

    Before:

    $ time cmake –build build -j24 real 3m26.932s

    After:

    $ time cmake –build build -j24 real 3m7.556s

    Generally they allow for jobservers (either make -jX or ninja) to be better utilized. This can be verified using top while building and looking at the number of compiles running at any given time before/after these changes. Before, it’s easy to observe periods of stalling when only one or two compiles are happening. After these changes, the compiler process count should mostly match the number of jobs given (-jX) until it falls off at the end.


    The first commit sets DEPENDS_EXPLICIT_ONLY for commands which generate our test header files. Without this option, test_bitcoin’s generated headers won’t be built until all of its other dependencies have been built. This introduces a significant stall in the build, though currently only Ninja benefits from this being set, and only CMake >= 3.27 understands it.

    Example from a generated build.ninja:

    Before:

    # Custom command for src/test/data/base58_encode_decode.json.h

    build src/test/data/base58_encode_decode.json.h | ${cmake_ninja_workdir}src/test/data/base58_encode_decode.json.h: CUSTOM_COMMAND /home/cory/dev/bitcoin/src/test/data/base58_encode_decode.json /home/cory/dev/bitcoin/cmake/script/GenerateHeaderFromJson.cmake || libcrc32c.a libcrc32c_sse42.a libleveldb.a libminisketch.a minisketch_clmul src/bitcoin_clientversion src/crypto/libbitcoin_crypto.a src/crypto/libbitcoin_crypto_avx2.a src/crypto/libbitcoin_crypto_sse41.a src/crypto/libbitcoin_crypto_x86_shani.a src/generate_build_info src/libbitcoin_cli.a src/libbitcoin_common.a src/libbitcoin_consensus.a src/libbitcoin_node.a src/secp256k1/src/libsecp256k1.a src/secp256k1/src/secp256k1_precomputed src/test/util/libtest_util.a src/univalue/libunivalue.a src/util/libbitcoin_util.a src/wallet/libbitcoin_wallet.a src/zmq/libbitcoin_zmq.a

    After:

    # Custom command for src/test/data/base58_encode_decode.json.h

    build src/test/data/base58_encode_decode.json.h | ${cmake_ninja_workdir}src/test/data/base58_encode_decode.json.h: CUSTOM_COMMAND /home/cory/dev/bitcoin/src/test/data/base58_encode_decode.json /home/cory/dev/bitcoin/cmake/script/GenerateHeaderFromJson.cmake


    The second commit is more significant. It sets CMAKE_OPTIMIZE_DEPENDENCIES globally, which allows the objects of static libs to be built in parallel when one lib depends on the other. This can be set as a per-lib property, ~but I don’t see any need for that as we don’t currently have any edge-cases where this wouldn’t be ok. If those should arise, we could always disable on a per-lib basis~.

    Edit: turns out this triggers an upstream bug, which I guess can be considered an edge-case until fixed in CMake. I’ve added 2 per-lib opt-outs as a result.

    Example:

    Before:

    # Link the static library src/libbitcoin_cli.a

    build src/libbitcoin_cli.a: CXX_STATIC_LIBRARY_LINKER__bitcoin_cli_RelWithDebInfo src/CMakeFiles/bitcoin_cli.dir/compat/stdin.cpp.o src/CMakeFiles/bitcoin_cli.dir/rpc/client.cpp.o || src/univalue/libunivalue.a

    After:

    # Link the static library src/libbitcoin_cli.a

    build src/libbitcoin_cli.a: CXX_STATIC_LIBRARY_LINKER__bitcoin_cli_RelWithDebInfo src/CMakeFiles/bitcoin_cli.dir/compat/stdin.cpp.o src/CMakeFiles/bitcoin_cli.dir/rpc/client.cpp.o

  2. DrahtBot commented at 6:44 pm on September 16, 2024: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK l0rinc, willcl-ark
    Concept ACK hebasto

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #30901 (cmake: Revamp handling of data files for {test,bench}_bitcoin targets by hebasto)
    • #28792 (Embed default ASMap as binary dump header file by fjahr)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  3. DrahtBot added the label Build system on Sep 16, 2024
  4. theuni requested review from hebasto on Sep 16, 2024
  5. DrahtBot added the label CI failed on Sep 16, 2024
  6. theuni commented at 7:46 pm on September 16, 2024: member

    Looks like this is hitting a CMake bug: https://gitlab.kitware.com/cmake/cmake/-/issues/24058

    Converting to draft while I investigate.

  7. theuni marked this as a draft on Sep 16, 2024
  8. theuni force-pushed on Sep 16, 2024
  9. willcl-ark commented at 9:22 pm on September 16, 2024: member

    Concept ACK.

    I noticed a bit of a “gap” in our compilation on master the other day."

    screenshot_20240916-221513

    This PR appears to fix it quite nicely:

    image

    I didn’t see quite the same speedup as you though, but will investigate and do some more thorough testing/review tomorrow.

    These tests were also using a default build, run using ninja with -j16.

  10. theuni force-pushed on Sep 16, 2024
  11. theuni force-pushed on Sep 16, 2024
  12. DrahtBot removed the label CI failed on Sep 16, 2024
  13. theuni marked this as ready for review on Sep 17, 2024
  14. theuni force-pushed on Sep 17, 2024
  15. maflcko added the label DrahtBot Guix build requested on Sep 17, 2024
  16. hebasto commented at 8:45 am on September 17, 2024: member

    Concept ACK.

    a39a915c226775396f6505efe77a4aba4d0ae3ad Did you consider setting CMAKE_ADD_CUSTOM_COMMAND_DEPENDS_EXPLICIT_ONLY globally?

    aaf30ef89465c18228b4e46fd1378b9087274cbe

    turns out this triggers an upstream bug, which I guess can be considered an edge-case until fixed in CMake. I’ve added 2 per-lib opt-outs as a result.

    This bug may also affect the crc32c library after switching to the upstream build system.

  17. maflcko commented at 8:49 am on September 17, 2024: member

    … though currently only Ninja benefits from this being set, and only CMake >= 3.27 understands it.

    I wonder if one CI task should be using Ninja (and cmake >= 3.27), if it isn’t too hard to implement. Otherwise this config will remain untested and errors may sneak in to the master branch, only being detected after merge.

  18. in cmake/module/GenerateHeaders.cmake:10 in aaf30ef894 outdated
     1@@ -2,12 +2,19 @@
     2 # Distributed under the MIT software license, see the accompanying
     3 # file COPYING or https://opensource.org/license/mit/.
     4 
     5+if(CMAKE_VERSION VERSION_GREATER_EQUAL 3.27)
     6+  set(DEPENDS_EXPLICIT_OPT DEPENDS_EXPLICIT_ONLY)
     7+else()
     8+  set(DEPENDS_EXPLICIT_OPT)
     9+endif()
    10+
    


    hebasto commented at 8:53 am on September 17, 2024:
    A note for reviewers. The Guix builds are using CMake 3.24.2 (the cmake-minimal package).
  19. hebasto commented at 12:14 pm on September 17, 2024: member

    … though currently only Ninja benefits from this being set, and only CMake >= 3.27 understands it.

    I wonder if one CI task should be using Ninja (and cmake >= 3.27), if it isn’t too hard to implement. Otherwise this config will remain untested and errors may sneak in to the master branch, only being detected after merge.

    Please consider #30915.

  20. theuni commented at 1:51 pm on September 17, 2024: member

    Concept ACK.

    a39a915 Did you consider setting CMAKE_ADD_CUSTOM_COMMAND_DEPENDS_EXPLICIT_ONLY globally?

    Yes, but I didn’t feel as comfortable setting this one globally due to our (for ex) phony deploy targets. I figured we may make some changes in the future that could introduce hard-to-track-down bugs. It’s clear to see that the header generation is self-contained, and afaik that’s the only place that would matter for build performance.

  21. DrahtBot commented at 2:00 pm on September 17, 2024: contributor

    Guix builds (on x86_64) [untrusted test-only build, possibly unsafe, not for production use]

    File commit 9f1aa88d4d9596aeb5220fa30ee3972294b62793(master) commit b492ec55637aacc0ab5c7905ef6a07d67ce19b34(master and this pull)
    SHA256SUMS.part 58b44f5adc71c2a4... 92c2d99d5c2c299b...
    *-aarch64-linux-gnu-debug.tar.gz 55850b93e85c3347... e02fea8f8e1ffe54...
    *-aarch64-linux-gnu.tar.gz fae846aaf9fea0b7... 79fac3429fc6e816...
    *-arm-linux-gnueabihf-debug.tar.gz 0426295872ec43a2... 47a0e42463683e95...
    *-arm-linux-gnueabihf.tar.gz 27a12872b8ada11a... e76b2e19a2001e4b...
    *-arm64-apple-darwin-unsigned.tar.gz 54300b0a29a7998b... 6c00a2baf4bc9c5e...
    *-arm64-apple-darwin-unsigned.zip 562e78cd7591cde8... ca69bb4613fae917...
    *-arm64-apple-darwin.tar.gz 6aa58a8f90301fa9... 7f8160bbfcef1738...
    *-powerpc64-linux-gnu-debug.tar.gz 6c157065ef9eda2d... 6a66652e72b552c1...
    *-powerpc64-linux-gnu.tar.gz 6cb89079528938d2... 354701aec723e97c...
    *-riscv64-linux-gnu-debug.tar.gz d8d21e764b475786... 3b45e46456511ddb...
    *-riscv64-linux-gnu.tar.gz 3d3b46cf6da3a0e1... 4c2f5127865699ae...
    *-x86_64-apple-darwin-unsigned.tar.gz fb603e19fe15e837... 75d9554b1c590680...
    *-x86_64-apple-darwin-unsigned.zip e7320795bfdf4091... 35b98fe93214dccb...
    *-x86_64-apple-darwin.tar.gz 35320ad7bed83b2d... dd77ff7b04a227ac...
    *-x86_64-linux-gnu-debug.tar.gz 1917ce819eb44d06... e5e7b908a9b9e832...
    *-x86_64-linux-gnu.tar.gz f643a9ee6827456b... a7d6fc4322776f68...
    *.tar.gz aca1d4d37426ef68... 11f85372ff9c79ed...
    guix_build.log 15b4512c52f2afb5... 838afc53bda90ced...
    guix_build.log.diff 3a2249ad9e4edb69...
  22. DrahtBot removed the label DrahtBot Guix build requested on Sep 17, 2024
  23. maflcko commented at 3:14 pm on September 17, 2024: member

    It’s clear to see that the header generation is self-contained, and afaik that’s the only place that would matter for build performance.

    Could rebase on current master, now that the header generation is 30x faster, after 2a0949f0977db2dbb7ac452e8d58d413b9526756 ? This should also change the benchmark in the pull description.

  24. build: speedup by avoiding unnecessary dependencies on generated headers
    This prevents the generation of these headers from also depending on the
    dependencies of the libs/binaries which consume them.
    
    Specifically, this prevents generated test headers (such as
    test/data/base58_encode_decode.json.h) from depending on the
    dependencies of test_bitcoin (libcrc32c.a libcrc32c_sse42.a libleveldb.a)
    
    Note that this is currently only relevant for Ninja.
    
    For more detail, see:
    https://cmake.org/cmake/help/latest/command/add_custom_command.html
    330d16e1aa
  25. build: speedup by simplifying dependency graph
    Allow the objects of static libs to be built in parallel rather than serially
    based on their dependency ordering.
    
    For more detail, see:
    https://cmake.org/cmake/help/latest/prop_tgt/OPTIMIZE_DEPENDENCIES.html
    42edb2f4a5
  26. theuni force-pushed on Sep 17, 2024
  27. fanquake referenced this in commit 6b97882ab5 on Sep 18, 2024
  28. l0rinc commented at 2:50 pm on September 19, 2024: contributor

    Now that #30888 is merged, the difference is not that dramatic: I’m measuring a 0.5% change:

    latest master 2x

    % ccache --clear && git clean -fxd && cmake -B build && time cmake --build build -j10

    cmake –build build -j10 994.42s user 91.78s system 832% cpu 2:10.47 total cmake –build build -j10 993.43s user 91.40s system 832% cpu 2:10.30 total

    rebased branch 2x

    % ccache --clear && git clean -fxd && cmake -B build && time cmake --build build -j10

    cmake –build build -j10 997.78s user 90.28s system 867% cpu 2:05.49 total cmake –build build -j10 998.51s user 90.64s system 867% cpu 2:05.54 total

    % cmake --version

    cmake version 3.30.2


    But conceptually it seems to be better than before so: ACK 42edb2f4a5900e0b2451a7446d58962c0bf0095d

  29. DrahtBot requested review from willcl-ark on Sep 19, 2024
  30. DrahtBot requested review from hebasto on Sep 19, 2024
  31. willcl-ark commented at 3:14 pm on September 23, 2024: member

    ACK 42edb2f4a5900e0b2451a7446d58962c0bf0095d

    I agree with @l0rinc that this seems conceptually nicer, and also see a much smaller time diff since 30888:

     0# master @ 33adc7521cc8bb24b941d959022b084002ba7c60
     1________________________________________________________
     2Executed in  181.29 secs    fish           external
     3   usr time   38.26 mins  214.00 micros   38.26 mins
     4   sys time    1.49 mins   55.00 micros    1.49 mins
     5
     6
     7# this PR rebased on master @ 33adc7521cc8bb24b941d959022b084002ba7c60
     8________________________________________________________
     9Executed in  179.09 secs    fish           external
    10   usr time   39.35 mins  202.00 micros   39.35 mins
    11   sys time    1.47 mins   56.00 micros    1.47 mins
    

    I also still see a “nicer” build process structure after this PR (using ninja tracing):

    image

  32. DrahtBot added the label CI failed on Oct 17, 2024
  33. DrahtBot removed the label CI failed on Oct 18, 2024
  34. in CMakeLists.txt:46 in 42edb2f4a5
    39@@ -40,6 +40,11 @@ set(CMAKE_TRY_COMPILE_PLATFORM_VARIABLES
    40   CMAKE_CXX_LINK_EXECUTABLE
    41 )
    42 
    43+# Flatten static lib dependencies.
    44+# Without this, if libfoo.a depends on libbar.a, libfoo's objects can't begin
    45+# to be compiled until libbar.a has been created.
    46+set(CMAKE_OPTIMIZE_DEPENDENCIES TRUE)
    


    hebasto commented at 12:52 pm on October 22, 2024:
    CMake has some variables/properties that must be set before the project() command. And the CMAKE_OPTIMIZE_DEPENDENCIES is not one of them. For the sake of clarity, I suggest to keep only necessary stuff before the project() command.
  35. hebasto commented at 12:53 pm on October 22, 2024: member

    Tested and benchmarked on Ubuntu 23.10 with CMake 3.27.4 using the following command:

    0$ rm -rf build && cmake -B build -G Ninja --preset dev-mode -DWITH_MULTIPROCESS=OFF -DWITH_CCACHE=OFF && time cmake --build build -j 16
    
    • The master branch @ 6fc4692797121b54de0c54e5b09ee47f322c038a:
    0real	6m44.237s
    
    • 330d16e1aa8fe8d7a5bb755189d0d2991fef8a43:
    0real	6m45.264s
    
    • 42edb2f4a5900e0b2451a7446d58962c0bf0095d
    0real	6m45.859s
    

    I’m OK with the flattened dependency graph, but the expected build time improvement is questionable. At least, the “speed up” phrase should be removed from the PR description and commit messages.

  36. DrahtBot requested review from hebasto on Oct 22, 2024
  37. edilmedeiros commented at 7:54 pm on October 23, 2024: contributor

    I noticed a bit of a “gap” in our compilation on master the other day."

    I have been noticing this “gap” all the time with cmake. I’ll check with this PR.


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-11-21 09:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me