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: member

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

    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 commented at 3:35 pm on June 24, 2019: contributor
    @MarcoFalke Good idea! Submitted as #16278 :-)
  7. DrahtBot added the label Needs rebase on Jun 25, 2019
  8. practicalswift force-pushed on Jun 25, 2019
  9. fanquake removed the label Needs rebase on Jun 25, 2019
  10. practicalswift force-pushed on Jun 25, 2019
  11. practicalswift force-pushed on Jun 25, 2019
  12. 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
  13. 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
  14. 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
  15. pull[bot] referenced this in commit 7400135b79 on Jun 27, 2019
  16. practicalswift force-pushed on Jun 27, 2019
  17. practicalswift force-pushed on Jun 27, 2019
  18. 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
  19. 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:

     0$ git grep -E '^#include <(core_read|core_write)\.h>' -- "*.cpp" "*.h" | \
 1    cut -f1 -d: | uniq -c | grep ' 1 '
 2      1 src/core_read.cpp
 3      1 src/core_write.cpp
 4      1 src/qt/transactiontablemodel.cpp
 5      1 src/rpc/blockchain.cpp
 6      1 src/rpc/net.cpp
 7      1 src/test/blockfilter_tests.cpp
 8      1 src/test/rpc_tests.cpp
 9      1 src/test/transaction_tests.cpp
10      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?
  20. 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 :-)

  21. DrahtBot added the label Needs rebase on Jun 28, 2019
  22. practicalswift force-pushed on Jun 28, 2019
  23. practicalswift force-pushed on Jun 28, 2019
  24. DrahtBot removed the label Needs rebase on Jun 28, 2019
  25. 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? :-)

  26. DrahtBot added the label Needs rebase on Jul 3, 2019
  27. practicalswift force-pushed on Jul 3, 2019
  28. practicalswift force-pushed on Jul 3, 2019
  29. practicalswift commented at 9:30 pm on July 3, 2019: contributor
    Rebased! :-)
  30. fanquake removed the label Needs rebase on Jul 4, 2019
  31. DrahtBot added the label Needs rebase on Jul 8, 2019
  32. practicalswift force-pushed on Jul 10, 2019
  33. practicalswift force-pushed on Jul 10, 2019
  34. DrahtBot removed the label Needs rebase on Jul 10, 2019
  35. practicalswift commented at 3:53 pm on July 10, 2019: contributor
    Rebased! :-)
  36. DrahtBot added the label Needs rebase on Jul 12, 2019
  37. practicalswift force-pushed on Jul 20, 2019
  38. practicalswift force-pushed on Jul 20, 2019
  39. DrahtBot removed the label Needs rebase on Jul 20, 2019
  40. practicalswift commented at 8:56 pm on July 21, 2019: contributor
    Rebased! :-)
  41. DrahtBot added the label Needs rebase on Jul 24, 2019
  42. build: Remove unused C++ standard library includes 8bda325b7f
  43. build: Remove unused C compatibility includes 2b204108d5
  44. practicalswift force-pushed on Jul 31, 2019
  45. DrahtBot removed the label Needs rebase on Jul 31, 2019
  46. build: Remove unused includes (project local headers and headers for external dependencies) 1f97bc924f
  47. Split core_io.h into the expected core_read.h/core_write.h (matching core_read.cpp/core_write.cpp) 523e40e3b7
  48. practicalswift force-pushed on Jul 31, 2019
  49. practicalswift closed this on Aug 14, 2019
  50. 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?

  51. MarcoFalke referenced this in commit 46d6930f8c on Oct 16, 2019
  52. sidhujag referenced this in commit 288cc1fa9f on Oct 18, 2019
  53. deadalnix referenced this in commit a3160568ef on Jun 15, 2020
  54. practicalswift deleted the branch on Apr 10, 2021
  55. vijaydasmp referenced this in commit 3531aa95e8 on Nov 23, 2021
  56. vijaydasmp referenced this in commit 4d1b3654aa on Nov 23, 2021
  57. vijaydasmp referenced this in commit bfb32d6b2d on Nov 24, 2021
  58. vijaydasmp referenced this in commit 3f839b0758 on Nov 25, 2021
  59. vijaydasmp referenced this in commit 89b2eccaa0 on Nov 26, 2021
  60. PastaPastaPasta referenced this in commit b0b8b2d2a9 on Nov 30, 2021
  61. PastaPastaPasta referenced this in commit 3c5dcb036a on Dec 13, 2021
  62. DrahtBot locked this on Aug 18, 2022


practicalswift DrahtBot hebasto MarcoFalke Sjors

Labels
Refactoring

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-17 09:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me