refactoring: Remove unused includes #16659

pull practicalswift wants to merge 1 commits into bitcoin:master from practicalswift:unused-includes changing 119 files +0 −159
  1. practicalswift commented at 4:28 pm on August 19, 2019: contributor

    As requested by MarcoFalke in #16273 (comment):

    This PR removes unused includes.

    Please note that in contrast to #16273 I’m limiting the scope to the trivial cases of pure removals (i.e. no includes added) to make reviewing easier.

    I’m seeking “Concept ACK”:s for this obviously non-urgent minor cleanup.

    Rationale:

    • Avoids unnecessary re-compiles in case of header changes.
    • Makes reasoning about code dependencies easier.
    • Reduces compile-time memory usage.
    • Reduces compilation time.
    • Warm fuzzy feeling of being lean :-)
  2. GChuf commented at 5:03 pm on August 19, 2019: contributor
    I’m all for lean and mean stuff. Concept ACK
  3. MarcoFalke added this to the milestone Future on Aug 19, 2019
  4. jb55 commented at 5:59 pm on August 19, 2019: member
    Concept ACK
  5. DrahtBot added the label Consensus on Aug 19, 2019
  6. DrahtBot added the label GUI on Aug 19, 2019
  7. DrahtBot added the label Mempool on Aug 19, 2019
  8. DrahtBot added the label Mining on Aug 19, 2019
  9. DrahtBot added the label P2P on Aug 19, 2019
  10. DrahtBot added the label Refactoring on Aug 19, 2019
  11. DrahtBot added the label RPC/REST/ZMQ on Aug 19, 2019
  12. DrahtBot added the label Tests on Aug 19, 2019
  13. DrahtBot added the label TX fees and policy on Aug 19, 2019
  14. DrahtBot added the label Utils/log/libs on Aug 19, 2019
  15. DrahtBot added the label UTXO Db and Indexes on Aug 19, 2019
  16. DrahtBot added the label Validation on Aug 19, 2019
  17. DrahtBot added the label Wallet on Aug 19, 2019
  18. fanquake removed the label Consensus on Aug 19, 2019
  19. fanquake removed the label GUI on Aug 19, 2019
  20. fanquake removed the label Mempool on Aug 19, 2019
  21. fanquake removed the label Mining on Aug 19, 2019
  22. fanquake removed the label P2P on Aug 19, 2019
  23. fanquake removed the label RPC/REST/ZMQ on Aug 19, 2019
  24. fanquake removed the label TX fees and policy on Aug 19, 2019
  25. fanquake removed the label Tests on Aug 19, 2019
  26. fanquake removed the label UTXO Db and Indexes on Aug 19, 2019
  27. fanquake removed the label Utils/log/libs on Aug 19, 2019
  28. fanquake removed the label Validation on Aug 19, 2019
  29. fanquake removed the label Wallet on Aug 19, 2019
  30. fanquake added the label Needs Conceptual Review on Aug 19, 2019
  31. DrahtBot commented at 6:28 pm on August 19, 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:

    • #16839 (Replace Connman and BanMan globals with Node local by ryanofsky)
    • #16528 ([WIP] Native Descriptor Wallets (take 2) by achow101)
    • #16341 (Introduce ScriptPubKeyMan interface and use it for key and script management (aka wallet boxes) by achow101)
    • #15934 (Merge settings one place instead of five places by ryanofsky)
    • #14053 (Add address-based index (attempt 4?) by marcinja)
    • #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.

  32. emilengler commented at 7:27 pm on August 19, 2019: contributor
    Concept ACK
  33. promag commented at 1:36 am on August 25, 2019: member
    Concept ACK.
  34. l2a5b1 commented at 8:42 am on August 29, 2019: contributor
    Concept ACK
  35. practicalswift commented at 8:23 am on September 26, 2019: contributor
    @MarcoFalke Could this one get a milestone? I think it should have a concrete milestone if valuable enough to keep open :) I’m trying to trim my set of open PR:s so if no milestone I’d rather close.
  36. promag commented at 10:41 am on September 26, 2019: member

    Running make on HEAD 150c289b4034fb2101df61998dd71b11f3d7fae1:

     0     1286.75 real      1170.31 user        99.80 sys
 1 748662784  maximum resident set size
 2         0  average shared memory size
 3         0  average unshared data size
 4         0  average unshared stack size
 5  35466407  page reclaims
 6    168811  page faults
 7         0  swaps
 8         0  block input operations
 9         0  block output operations
10         0  messages sent
11         0  messages received
12      2512  signals received
13     39561  voluntary context switches
14    377820  involuntary context switches

    And on HEAD^ e00ecb3d7aaee463643e486ca03c318e192b8058:

     0     1440.06 real      1300.06 user       113.79 sys
 1 747356160  maximum resident set size
 2         0  average shared memory size
 3         0  average unshared data size
 4         0  average unshared stack size
 5  35429838  page reclaims
 6    212723  page faults
 7         0  swaps
 8         0  block input operations
 9         0  block output operations
10         0  messages sent
11         0  messages received
12      2381  signals received
13     55950  voluntary context switches
14    932017  involuntary context switches

    (make clean && ccache -C before each build).

    So I’d ACK if this was already rebased and non draft.

  37. practicalswift commented at 10:48 am on September 26, 2019: contributor
    @promag Thanks a lot for benchmarking! I’ll update and rebase as soon as I get clarification from @MarcoFalke regarding the milestone :)
  38. practicalswift commented at 8:53 am on October 10, 2019: contributor
    @MarcoFalke Friendly ping :)
  39. laanwj commented at 12:20 pm on October 10, 2019: member

    A PR only removing lines is very good. I also think early in the 0.20 cycle is a good time to do this.

    So to be clear: this removes unnecessary includes that would otherwise cause the file to be included unnecessary, not includes that still end up being included indirectly through another route?

  40. practicalswift commented at 1:49 pm on October 10, 2019: contributor

    @laanwj Thanks for the clarification!

    So to be clear: this removes unnecessary includes that would otherwise cause the file to be included unnecessary, not includes that still end up being included indirectly through another route?

    Yes, exactly!

    Would you mind adding the 0.20 tag? I’ll update the PR :)

  41. laanwj removed this from the milestone Future on Oct 10, 2019
  42. laanwj added this to the milestone 0.20.0 on Oct 10, 2019
  43. MarcoFalke marked this as ready for review on Oct 11, 2019
  44. DrahtBot added the label Needs rebase on Oct 11, 2019
  45. laanwj commented at 7:15 am on October 12, 2019: member
    Let’s get this merged soon then, to not have to keep rebasing, at least there’s basically zero risk.
  46. practicalswift force-pushed on Oct 12, 2019
  47. DrahtBot removed the label Needs rebase on Oct 12, 2019
  48. practicalswift force-pushed on Oct 12, 2019
  49. practicalswift force-pushed on Oct 14, 2019
  50. practicalswift force-pushed on Oct 14, 2019
  51. practicalswift force-pushed on Oct 14, 2019
  52. DrahtBot added the label Needs rebase on Oct 15, 2019
  53. practicalswift force-pushed on Oct 15, 2019
  54. Remove unused includes 084e17cebd
  55. practicalswift force-pushed on Oct 15, 2019
  56. DrahtBot removed the label Needs rebase on Oct 15, 2019
  57. practicalswift commented at 5:41 am on October 16, 2019: contributor

    Rebased again :)

    I think this one should be ready for final code review!

  58. ryanofsky approved
  59. ryanofsky commented at 2:30 pm on October 16, 2019: member

    Code review ACK 084e17cebd424b8e8ced674bc810eef4e6ee5d3b. PR only removes include lines and it still compiles. In the worst case someone might have to explicitly add an include later for something now included implicitly. But maybe some effort was taken to avoid this, and it wouldn’t be a tragedy anyway.

    I would encourage @practicalswift to be more transparent about tools and methodologies used, since it’s unclear how this PR was generated, how the changes should be maintained, and if there were tools used that might deserve credit.

  60. practicalswift commented at 3:29 pm on October 16, 2019: contributor

    @ryanofsky The only tools used are nm and cxxfilt (python module). The rest is a very much Bitcoin Core specific Python script I’ve written myself :)

    0$ wc -l find_redundant_includes.??
1 2207 find_redundant_includes.py
2    3 find_redundant_includes.sh
3 2210 total
4$ grep -v '"' find_redundant_includes.py | wc -l
5371

    2207 LOC might sound crazy but a lot of it is taken up by a.) a mapping between object file names and headers file names and b.) a mapping between standard library headers and standard library functions/symbols. The real line count should be somewhere around 400 LOC.

    It is not pretty but it works :)

  61. ryanofsky commented at 3:55 pm on October 16, 2019: member
    Thanks. I’d be curious to see the script if you want to post it somewhere like a gist.
  62. MarcoFalke referenced this in commit 46d6930f8c on Oct 16, 2019
  63. MarcoFalke merged this on Oct 16, 2019
  64. MarcoFalke closed this on Oct 16, 2019
  65. in src/node/transaction.cpp:9 in 084e17cebd 
    5@@ -6,7 +6,6 @@
6 #include <consensus/validation.h>
7 #include <net.h>
8 #include <net_processing.h>
9-#include <txmempool.h>
    MarcoFalke commented at 9:44 pm on October 16, 2019:
    This is actually used
    practicalswift commented at 9:46 pm on October 16, 2019:
    Oh, I’ll investigate shortly! Did you find any other examples? Was it found by manual review?
  66. MarcoFalke commented at 9:44 pm on October 16, 2019: member
    ACK
  67. practicalswift commented at 11:07 pm on October 16, 2019: contributor

    Thanks for merging!

    See below for a script run after this merge.

    I believe the remaining cases to be of the “non-trivial” type where the header directly included is not needed in itself but one of more of the transitively included headers are.

    Consider the src/bench/rpc_blockchain.cpp example: we are currently including validation.h which transitively gives us PROTOCOL_VERSION, CBlock, CBlockIndex and uint256. But we don’t need anything validation.h provides in itself.

    Instead of including validation.h we should include version.h for PROTOCOL_VERSION, primitives/block.h for CBlock, chain.h for CBlockIndex and uint256.h for uint256.

    Like this:

     0diff --git a/src/bench/rpc_blockchain.cpp b/src/bench/rpc_blockchain.cpp
 1index 2fc6f116a..00cec5ecb 100644
 2--- a/src/bench/rpc_blockchain.cpp
 3+++ b/src/bench/rpc_blockchain.cpp
 4@@ -5,9 +5,12 @@
 5 #include <bench/bench.h>
 6 #include <bench/data.h>
 7 
 8-#include <validation.h>
 9+#include <chain.h>
10+#include <primitives/block.h>
11 #include <streams.h>
12 #include <rpc/blockchain.h>
13+#include <uint256.h>
14+#include <version.h>
15 
16 #include <univalue.h>

     0$ ./find_redundant_includes.sh
 1src/bench/rpc_blockchain.cpp -- unused include: validation.h
 2src/bench/verify_script.cpp -- unused include: script/standard.h
 3src/coins.h -- unused include: core_memusage.h
 4src/core_read.cpp -- unused include: script/sign.h
 5src/crypto/hkdf_sha256_32.h -- unused include: crypto/hmac_sha256.h
 6src/index/txindex.h -- unused include: txdb.h
 7src/net.h -- unused include: addrdb.h
 8src/net.h -- unused include: limitedmap.h
 9src/net.h -- unused include: policy/feerate.h
10src/noui.cpp -- unused include: util/system.h
11src/psbt.h -- unused include: node/transaction.h
12src/psbt.h -- unused include: optional.h
13src/psbt.h -- unused include: policy/feerate.h
14src/qt/bantablemodel.h -- unused include: net.h
15src/qt/receivecoinsdialog.cpp -- unused include: wallet/wallet.h
16src/qt/sendcoinsdialog.cpp -- unused include: txmempool.h
17src/qt/sendcoinsdialog.cpp -- unused include: wallet/fees.h
18src/qt/signverifymessagedialog.cpp -- unused include: wallet/wallet.h
19src/qt/walletmodel.h -- unused include: script/standard.h
20src/qt/winshutdownmonitor.cpp -- unused include: util/system.h
21src/rpc/misc.cpp -- unused include: rpc/blockchain.h
22src/rpc/util.h -- unused include: node/transaction.h
23src/rpc/util.h -- unused include: script/sign.h
24src/script/sign.h -- unused include: streams.h
25src/support/events.h -- unused include: ios
26src/sync.h -- unused include: thread
27src/test/fuzz/FuzzedDataProvider.h -- unused include: utility
28src/test/fuzz/eval_script.cpp -- unused include: limits
29src/test/key_tests.cpp -- unused include: util/system.h
30src/txmempool.h -- unused include: random.h
31src/uint256.h -- unused include: assert.h
32src/util/system.h -- unused include: util/memory.h
33src/util/system.h -- unused include: util/time.h
34src/wallet/coinselection.h -- unused include: random.h
35src/wallet/crypter.h -- unused include: script/signingprovider.h
36src/wallet/test/wallet_tests.cpp -- unused include: rpc/server.h
37src/wallet/wallet.cpp -- unused include: util/validation.h
38src/wallet/walletdb.h -- unused include: script/sign.h
39src/zmq/zmqabstractnotifier.h -- unused include: zmq/zmqconfig.h
40src/zmq/zmqconfig.h -- unused include: primitives/transaction.h
41src/zmq/zmqconfig.h -- unused include: stdarg.h

    Anyone who wants to address the above cases too? Then we should be as lean as it gets when it comes to redundant includes :)

    If you do: let me know if you find any false positives/false negatives given the list above.

    One caveat is that I’m currently skipping analysis for core_io.h due to the weird naming mismatch between core_io.h ⇄ core_write.cpp/core_read.cpp :)

  68. laanwj commented at 8:58 am on October 17, 2019: member

    One caveat is that I’m currently skipping analysis for core_io.h

    It suppose that is a trivial header anyway, memory usage wise. It’s a few simple functions. It doesn’t pull in any boost nor non-run-of-the-mill C++ standard library headers.

  69. sidhujag referenced this in commit 288cc1fa9f on Oct 18, 2019
  70. deadalnix referenced this in commit 2e7115ee28 on May 6, 2020
  71. deadalnix referenced this in commit a3160568ef on Jun 15, 2020
  72. fanquake referenced this in commit e658b0e49b on Mar 27, 2021
  73. sidhujag referenced this in commit adcbe1adaf on Mar 27, 2021
  74. practicalswift deleted the branch on Apr 10, 2021
  75. PastaPastaPasta referenced this in commit 579ff9f89d on Jun 27, 2021
  76. PastaPastaPasta referenced this in commit 803786cb8b on Jun 28, 2021
  77. PastaPastaPasta referenced this in commit 67dd77e43b on Jun 29, 2021
  78. PastaPastaPasta referenced this in commit 3c5dcb036a on Dec 13, 2021
  79. str4d referenced this in commit c4e9e1c134 on Jul 16, 2022
  80. DrahtBot locked this on Aug 18, 2022


practicalswift GChuf jb55 DrahtBot emilengler promag l2a5b1 laanwj ryanofsky MarcoFalke

Labels
Refactoring Needs Conceptual Review

Milestone
0.20.0

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