refactor: Remove unused includes #16273

pull practicalswift wants to merge 4 commits into bitcoin:master from practicalswift:cut-compilation-bloat changing 168 files +163 −262
  1. practicalswift commented at 8:46 PM on June 23, 2019: contributor

    Reduce compile-time memory usage by 2%, compilation time by 2% and avoid unnecessary recompiles by removing unused includes.

    The total accumulated max RSS usage is reduced by 2% (roughly -3564 MB, low variance measurement).

    The total compilation time is reduced by 2% (roughly -73 seconds in user CPU time, slightly higher variance measurement).

    Commits:

    • Reduce memory usage during build by not including unused C++ standard library headers
    • Reduce memory usage during build by not including unused C compatibility headers
    • Reduce memory usage during build by not including unused headers (project local headers and headers for external dependencies)
    • Minimise includes and simplify header analysis/reasoning by splitting the core_io.h into the expected core_read.h/core_write.h (matching core_read.cpp/core_write.cpp)

    The two first commits account for half of the speedup and the third commit account for the other half.

    This PR is a follow-up to the merged PR #16129 which reduced max RSS and compilation time by 2%.

    Comparing max memory usage (RSS) between old revision 2cbcc55ba6aea26d64eae3981b83dac04f70240f (branch master) and new revision fd2f3033591a370438cf2c15f26f28a6608238b5 (branch cut-compilation-bloat):

    File Old New Diff Percent
    bench/bench_bench_bitcoin-bench_bitcoin.o 235 MB 218 MB -18 MB -7 %
    bench/bench_bench_bitcoin-verify_script.o 218 MB 178 MB -39 MB -18 %
    libbitcoin_common_a-netbase.o 281 MB 263 MB -17 MB -6 %
    libbitcoin_common_a-protocol.o 236 MB 218 MB -18 MB -8 %
    libbitcoin_common_a-warnings.o 212 MB 194 MB -17 MB -8 %
    libbitcoin_server_a-addrman.o 286 MB 269 MB -17 MB -6 %
    libbitcoin_server_a-banman.o 253 MB 235 MB -17 MB -7 %
    libbitcoin_server_a-dbwrapper.o 287 MB 270 MB -17 MB -6 %
    libbitcoin_server_a-flatfile.o 239 MB 221 MB -18 MB -7 %
    libbitcoin_server_a-httprpc.o 398 MB 363 MB -35 MB -9 %
    libbitcoin_server_a-timedata.o 257 MB 239 MB -18 MB -7 %
    libbitcoin_server_a-torcontrol.o 604 MB 504 MB -99 MB -16 %
    libbitcoin_util_a-chainparamsbase.o 225 MB 208 MB -17 MB -8 %
    qt/qt_libbitcoinqt_a-bitcoin.o 595 MB 498 MB -97 MB -16 %
    qt/qt_libbitcoinqt_a-moc_bantablemodel.o 304 MB 199 MB -105 MB -35 %
    qt/qt_libbitcoinqt_a-receivecoinsdialog.o 585 MB 471 MB -114 MB -19 %
    qt/qt_libbitcoinqt_a-sendcoinsdialog.o 809 MB 706 MB -103 MB -13 %
    qt/qt_libbitcoinqt_a-signverifymessagedialog.o 620 MB 400 MB -220 MB -36 %
    qt/test/qt_test_test_bitcoin_qt-moc_rpcnestedtests.o 438 MB 145 MB -293 MB -67 %
    qt/test/qt_test_test_bitcoin_qt-test_main.o 539 MB 438 MB -102 MB -19 %
    rpc/libbitcoin_cli_a-client.o 263 MB 246 MB -17 MB -6 %
    rpc/libbitcoin_common_a-rawtransaction_util.o 423 MB 395 MB -28 MB -7 %
    rpc/libbitcoin_common_a-util.o 408 MB 384 MB -23 MB -6 %
    rpc/libbitcoin_util_a-protocol.o 258 MB 241 MB -17 MB -7 %
    test/bench_bench_bitcoin-util.o 613 MB 518 MB -95 MB -15 %
    test/test_test_bitcoin-bech32_tests.o 541 MB 513 MB -28 MB -5 %
    test/test_test_bitcoin-fs_tests.o 540 MB 513 MB -27 MB -5 %
    test/test_test_bitcoin-reverselock_tests.o 549 MB 520 MB -29 MB -5 %
    util/libbitcoin_util_a-error.o 220 MB 202 MB -18 MB -8 %
    util/libbitcoin_util_a-moneystr.o 108 MB 81 MB -27 MB -25 %
    util/libbitcoin_util_a-threadnames.o 69 MB 48 MB -21 MB -31 %
    wallet/libbitcoin_wallet_a-walletutil.o 249 MB 232 MB -17 MB -7 %
    zmq/libbitcoin_zmq_a-zmqrpc.o 347 MB 326 MB -21 MB -6 %
    ... suppressing 421 unchanged measurements ... ... ... ... ...
    Average (N=454) 321 MB 313 MB -8 MB -2 %
    Sum (N=454) 145549 MB 141986 MB -3564 MB -2 %

    Comparing build time (user CPU time) between old revision 2cbcc55ba6aea26d64eae3981b83dac04f70240f (branch master) and new revision fd2f3033591a370438cf2c15f26f28a6608238b5 (branch cut-compilation-bloat), listing filenames and individual results only for files with a measured change in max memory usage (RSS) to reduce noise:

    File Old New Diff Percent
    bench/bench_bench_bitcoin-bench_bitcoin.o 3 s. 3 s. -0 s. -5 %
    bench/bench_bench_bitcoin-verify_script.o 4 s. 3 s. -1 s. -15 %
    libbitcoin_common_a-netbase.o 6 s. 6 s. -0 s. -1 %
    libbitcoin_common_a-protocol.o 4 s. 4 s. -0 s. -7 %
    libbitcoin_common_a-warnings.o 3 s. 3 s. -0 s. -9 %
    libbitcoin_server_a-addrman.o 5 s. 5 s. -0 s. -3 %
    libbitcoin_server_a-banman.o 4 s. 4 s. -0 s. -0 %
    libbitcoin_server_a-dbwrapper.o 6 s. 6 s. -0 s. -1 %
    libbitcoin_server_a-flatfile.o 4 s. 4 s. -0 s. -1 %
    libbitcoin_server_a-httprpc.o 9 s. 8 s. -1 s. -8 %
    libbitcoin_server_a-timedata.o 4 s. 4 s. -0 s. -2 %
    libbitcoin_server_a-torcontrol.o 15 s. 12 s. -2 s. -17 %
    libbitcoin_util_a-chainparamsbase.o 3 s. 3 s. -0 s. -6 %
    qt/qt_libbitcoinqt_a-bitcoin.o 13 s. 10 s. -3 s. -25 %
    qt/qt_libbitcoinqt_a-moc_bantablemodel.o 4 s. 2 s. -1 s. -36 %
    qt/qt_libbitcoinqt_a-receivecoinsdialog.o 11 s. 8 s. -3 s. -31 %
    qt/qt_libbitcoinqt_a-sendcoinsdialog.o 16 s. 14 s. -1 s. -7 %
    qt/qt_libbitcoinqt_a-signverifymessagedialog.o 12 s. 8 s. -4 s. -36 %
    qt/test/qt_test_test_bitcoin_qt-moc_rpcnestedtests.o 6 s. 2 s. -5 s. -72 %
    qt/test/qt_test_test_bitcoin_qt-test_main.o 10 s. 7 s. -3 s. -28 %
    rpc/libbitcoin_cli_a-client.o 4 s. 4 s. -0 s. -3 %
    rpc/libbitcoin_common_a-rawtransaction_util.o 9 s. 9 s. -0 s. -3 %
    rpc/libbitcoin_common_a-util.o 9 s. 8 s. -0 s. -2 %
    rpc/libbitcoin_util_a-protocol.o 5 s. 5 s. -0 s. -1 %
    test/bench_bench_bitcoin-util.o 13 s. 9 s. -4 s. -31 %
    test/test_test_bitcoin-bech32_tests.o 11 s. 11 s. -1 s. -5 %
    test/test_test_bitcoin-fs_tests.o 11 s. 11 s. -1 s. -5 %
    test/test_test_bitcoin-reverselock_tests.o 12 s. 11 s. -1 s. -6 %
    util/libbitcoin_util_a-error.o 3 s. 3 s. -0 s. -4 %
    util/libbitcoin_util_a-moneystr.o 2 s. 1 s. -0 s. -24 %
    util/libbitcoin_util_a-threadnames.o 1 s. 0 s. -0 s. -35 %
    wallet/libbitcoin_wallet_a-walletutil.o 4 s. 4 s. -0 s. -4 %
    zmq/libbitcoin_zmq_a-zmqrpc.o 6 s. 5 s. -1 s. -9 %
    ... suppressing 421 measurements ... ... ... ... ...
    Average (N=454) 7 s. 6 s. -0 s. -2 %
    Sum (N=454) 2994 s. 2921 s. -73 s. -2 %
  2. fanquake added the label Refactoring on Jun 23, 2019
  3. DrahtBot commented at 10:18 PM on June 23, 2019: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #16440 (BIP-322: Generic signed message format by kallewoof)
    • #16341 (Introduce ScriptPubKeyMan interface and use it for key and script management (aka wallet boxes) by achow101)
    • #16324 (Get cs_main out of the critical path in ProcessMessages by TheBlueMatt)
    • #16323 (Call ProcessNewBlock() asynchronously by TheBlueMatt)
    • #16248 (Make whitebind/whitelist permissions more flexible by NicolasDorier)
    • #16224 (gui: Bilingual GUI error messages by hebasto)
    • #15934 (Separate settings merging from parsing by ryanofsky)
    • #15382 ([util] add runCommandParseJSON by Sjors)
    • #15294 ([moveonly] wallet: Extract RipeMd160 by Empact)
    • #14046 (net: Refactor message parsing (CNetMessage), adds flexibility by jonasschnelli)
    • #13789 (crypto/sha256: Use pragmas to enforce necessary intrinsics for GCC and Clang by luke-jr)
    • #13686 (ZMQ: Small cleanups in the ZMQ code by domob1812)

    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.

  4. hebasto commented at 10:43 AM on June 24, 2019: member

    Just wondering what is a proper way to test such PRs?

  5. MarcoFalke commented at 1:59 PM on June 24, 2019: member

    Could create a separate pull for all changes in src/bench/ src/test src/wallet/test/?

  6. practicalswift cross-referenced this on Jun 24, 2019 from issue tests: Remove unused includes by practicalswift
  7. practicalswift commented at 3:35 PM on June 24, 2019: contributor

    @MarcoFalke Good idea! Submitted as #16278 :-)

  8. DrahtBot added the label Needs rebase on Jun 25, 2019
  9. practicalswift force-pushed on Jun 25, 2019
  10. fanquake removed the label Needs rebase on Jun 25, 2019
  11. practicalswift force-pushed on Jun 25, 2019
  12. practicalswift force-pushed on Jun 25, 2019
  13. practicalswift renamed this:
    refactor: Reduce total compilation time by 2% and avoid unnecessary recompiles by removing unused includes
    refactor: Reduce compile-time memory usage by 2 %, compilation time by 2% and avoid unnecessary recompiles by removing unused includes
    on Jun 27, 2019
  14. practicalswift renamed this:
    refactor: Reduce compile-time memory usage by 2 %, compilation time by 2% and avoid unnecessary recompiles by removing unused includes
    refactor: Reduce compile-time memory usage by 2%, compilation time by 2% and avoid unnecessary recompiles by removing unused includes
    on Jun 27, 2019
  15. practicalswift cross-referenced this on Jun 27, 2019 from issue Building requires >1GB memory by TheBlueMatt
  16. Sjors commented at 1:08 PM on June 27, 2019: member

    Good idea to move the tests to #16027. The commit splitting core_io.h into the ex… …pected core_read.h/core_write.h could also be a separate PR if this ends up taking longer.

    This touches quite a few other PRs, but I suspect most won't trigger a rebase. Otherwise it would be best to merge this right after a major release (@MarcoFalke would be cool if @DrahtBot could tell). So concept ACK.

    Suggest moving "Reduce memory usage during build by" from the commit name(s) to the commit message(s); emphasize what you're changing in the title, and why in the message.

    I was able to run unit and functional* tests on macOS with 67fc141 (rebased on on master).

    • except p2p_invalid_messages.py as usual
  17. pull[bot] referenced this in commit 7400135b79 on Jun 27, 2019
  18. practicalswift force-pushed on Jun 27, 2019
  19. practicalswift force-pushed on Jun 27, 2019
  20. MarcoFalke renamed this:
    refactor: Reduce compile-time memory usage by 2%, compilation time by 2% and avoid unnecessary recompiles by removing unused includes
    refactor: Remove unused includes
    on Jun 27, 2019
  21. in src/Makefile.am:130 in 50730e6318 outdated 
     124 | @@ -125,8 +125,9 @@ BITCOIN_CORE_H = \
 125 |    consensus/consensus.h \
 126 |    consensus/tx_check.h \
 127 |    consensus/tx_verify.h \
 128 | -  core_io.h \
 129 |    core_memusage.h \
 130 | +  core_read.h \
 131 | +  core_write.h \
    MarcoFalke commented at 3:42 PM on June 27, 2019:

    Why don't you keep the header and make this core_io.cpp?

    practicalswift commented at 4:02 PM on June 27, 2019:

    That is an option but it would introduce 18 (9 * 2) additional unused includes. Is it worth that cost?

    Reasoning:

    core_write.h includes string, amount.h and stdint.h (via amount.h).

    core_read.h includes string, vector and attributes.h.

    The following files include only one of core_read.h and core_write.h:

    $ git grep -E '^#include <(core_read|core_write)\.h>' -- "*.cpp" "*.h" | \
    cut -f1 -d: | uniq -c | grep ' 1 '
      1 src/core_read.cpp
      1 src/core_write.cpp
      1 src/qt/transactiontablemodel.cpp
      1 src/rpc/blockchain.cpp
      1 src/rpc/net.cpp
      1 src/test/blockfilter_tests.cpp
      1 src/test/rpc_tests.cpp
      1 src/test/transaction_tests.cpp
      1 src/wallet/rpcdump.cpp
    Sjors commented at 6:09 PM on June 27, 2019:

    These seem like 18 very small includes. I'm fine with either splitting in read/write or having one io.

    MarcoFalke commented at 6:53 PM on June 27, 2019:

    Agree that they are not worth the effort, also you can avoid them by putting using CAmount=... instead of the full header.

    practicalswift commented at 7:03 PM on June 27, 2019:

    I don't feel strongly about this. I'll let others chime in and update according to the consensus opinion. @MarcoFalke I take it I have your Concept ACK? Mind making it explicit in a standalone comment?

  22. practicalswift commented at 4:05 PM on June 27, 2019: contributor

    @Sjors Thanks for taking the time to review and test!

    Good point about the commit messages! I've now changed that. I guess I was eager to explain the why to also reach those who form their judgement based on PR metadata alone: the GitHub equivalent of having an opinion about the content of a news article after only reading the title :-)

  23. DrahtBot added the label Needs rebase on Jun 28, 2019
  24. practicalswift force-pushed on Jun 28, 2019
  25. practicalswift force-pushed on Jun 28, 2019
  26. DrahtBot removed the label Needs rebase on Jun 28, 2019
  27. practicalswift commented at 10:11 AM on July 2, 2019: contributor

    Just wondering what is a proper way to test such PRs?

    Making sure we compile and pass the test suite (both functional and unit tests) on supported platforms would be the obvious starting point for testing I guess? :-)

  28. DrahtBot added the label Needs rebase on Jul 3, 2019
  29. practicalswift force-pushed on Jul 3, 2019
  30. practicalswift force-pushed on Jul 3, 2019
  31. practicalswift commented at 9:30 PM on July 3, 2019: contributor

    Rebased! :-)

  32. fanquake removed the label Needs rebase on Jul 4, 2019
  33. DrahtBot added the label Needs rebase on Jul 8, 2019
  34. practicalswift force-pushed on Jul 10, 2019
  35. practicalswift force-pushed on Jul 10, 2019
  36. DrahtBot removed the label Needs rebase on Jul 10, 2019
  37. practicalswift commented at 3:53 PM on July 10, 2019: contributor

    Rebased! :-)

  38. DrahtBot added the label Needs rebase on Jul 12, 2019
  39. practicalswift force-pushed on Jul 20, 2019
  40. practicalswift force-pushed on Jul 20, 2019
  41. DrahtBot removed the label Needs rebase on Jul 20, 2019
  42. practicalswift commented at 8:56 PM on July 21, 2019: contributor

    Rebased! :-)

  43. DrahtBot added the label Needs rebase on Jul 24, 2019
  44. build: Remove unused C++ standard library includes 8bda325b7f
  45. build: Remove unused C compatibility includes 2b204108d5
  46. practicalswift force-pushed on Jul 31, 2019
  47. DrahtBot removed the label Needs rebase on Jul 31, 2019
  48. build: Remove unused includes (project local headers and headers for external dependencies) 1f97bc924f
  49. Split core_io.h into the expected core_read.h/core_write.h (matching core_read.cpp/core_write.cpp) 523e40e3b7
  50. practicalswift force-pushed on Jul 31, 2019
  51. practicalswift closed this on Aug 14, 2019
  52. MarcoFalke commented at 5:03 PM on August 14, 2019: member

    @practicalswift While this is probably nothing to be merged as-is, I still liked having the pull request open. You might have noticed that I fixed the includes in files that I touched based on the diff in this pull. Would you mind re-opening and/or sharing the steps you took to generate this list?

    I tried to run iwyu once, but I wasn't really successful. So if your solution works better, why not put a comment in #15442?

  53. practicalswift cross-referenced this on Aug 19, 2019 from issue refactoring: Remove unused includes by practicalswift
  54. MarcoFalke referenced this in commit 46d6930f8c on Oct 16, 2019
  55. sidhujag referenced this in commit 288cc1fa9f on Oct 18, 2019
  56. deadalnix referenced this in commit a3160568ef on Jun 15, 2020
  57. practicalswift deleted the branch on Apr 10, 2021
  58. vijaydasmp referenced this in commit 3531aa95e8 on Nov 23, 2021
  59. vijaydasmp referenced this in commit 4d1b3654aa on Nov 23, 2021
  60. vijaydasmp referenced this in commit bfb32d6b2d on Nov 24, 2021
  61. vijaydasmp referenced this in commit 3f839b0758 on Nov 25, 2021
  62. vijaydasmp referenced this in commit 89b2eccaa0 on Nov 26, 2021
  63. PastaPastaPasta referenced this in commit b0b8b2d2a9 on Nov 30, 2021
  64. PastaPastaPasta referenced this in commit 3c5dcb036a on Dec 13, 2021
  65. bitcoin locked this on Aug 18, 2022
Contributors
practicalswiftDrahtBothebastoMarcoFalkeSjors
Labels
Refactoring
Linked (view graph)
#6658 Building requires >1GB memory#15442 Running iwyu on Bitcoin Core#16027 client 0.18.0 crashes when computer wakes up from hibernation#16129 refactor: Remove unused includes#16278 tests: Remove unused includes#16659 refactoring: Remove unused includes
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: 2026-05-20 06:54 UTC