refactor: Nuke policy/fees->mempool circular dependencies

hebasto commented at 5:08 pm on December 21, 2019: member

This PR:

gets rid of the policy/fees -> txmempool -> policy/fees circular dependency
is an alternative to #13949, which nukes only one circular dependency

fanquake added the label Refactoring on Dec 21, 2019

hebasto commented at 5:09 pm on December 21, 2019: member

Related discussion:

#15638 (review) by @ryanofsky
#13949 (comment) by @MarcoFalke

DrahtBot commented at 7:04 pm on December 21, 2019: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	ryanofsky, theStack, glozow
Concept ACK	practicalswift, JeremyRubin, Empact
Stale ACK	promag

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26153 (Reduce wasted pseudorandom bytes in ChaCha20 + various improvements by sipa)
#25361 (BIP324: Cipher suite by dhruv)
#25325 (Add pool based memory resource by martinus)
#24545 (BIP324: Enable v2 P2P encrypted transport by dhruv)
#23962 (Use int32_t type for transaction size/weight consistently by hebasto)
#23561 (BIP324: Handshake prerequisites by dhruv)
#23233 (BIP324: Add encrypted p2p transport {de}serializer by dhruv)

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.

promag commented at 7:30 pm on December 21, 2019: member

Concept ACK.

promag commented at 12:09 pm on December 22, 2019: member

ACK fbf6b7524eac8a7668e49b7c340d06447a9d248d.

I’d squash 1st and 2nd commits. 3rd commit is unrelated but is a nice cleanup.

practicalswift commented at 12:39 pm on December 22, 2019: contributor

Concept ACK: thanks for nuking circular dependencies

hebasto force-pushed on Dec 22, 2019

hebasto commented at 12:47 pm on December 22, 2019: member

@promag

I’d squash 1st and 2nd commits. 3rd commit is unrelated but is a nice cleanup.

Done.

hebasto commented at 1:16 pm on December 22, 2019: member

~I stumble on UB sanitizer. How could a move-only change cause an error?~

~Is this a side effect of circular dependencies resolving?~ nm

hebasto force-pushed on Dec 22, 2019

hebasto commented at 2:42 pm on December 22, 2019: member

Travis is happy now ;)

JeremyRubin commented at 3:32 pm on January 17, 2020: contributor

Generally agree with this change, I’ve thought about doing it myself a few times myself. It also seems to be a good alternative to the observer pattern mentioned as we change far fewer things to get the same circular dependency break. So a slight code review ACK that this seems correct.

Slightly short of concept ack for a few reasons:

Needs a simple benchmark to show that we haven’t made functions calling the CTxMemPoolEntry’s non-header functions slower because they are no longer within the same compilation unit
Some other changes I’ve been working on lately w.r.t. epoch mempool introduce some new classes & move code between the CTxMemPool and CTxMemPoolEntry classes; while those can be rebased, it’s not clear where some of the shared code should live (perhaps in a third additional header, or a new circular dependency). We already have some things which are inherent circular dependencies (that won’t get picked up by the linter, maps of MemPoolChildren/MemPoolParents) that will deliver a big performance win to get rid of (I think it will at least), so I’d rather kill those circular dependencies first so that it doesn’t become harder to do later – I’m happy to open up a PR for that once the first step of my #17268 Project, #17925, goes through.

hebasto commented at 12:07 pm on January 22, 2020: member

@JeremyRubin

… to show that we haven’t made functions calling the CTxMemPoolEntry’s non-header functions slower because they are no longer within the same compilation unit

How is it possible?

JeremyRubin commented at 5:45 pm on January 22, 2020: contributor

It’s annoying, but because these functions were previously in the same translation unit then it’s possible they can get completely inlined, but now they’re in separate units they will definitely trigger function calls.

Because these updater functions get called in hot loops you have possibly replaced something optimized previously with something slow.

This matters a bit because we may expect these loops/regions to get even hotter if we increase mempool limits.

One way of addressing this would be to make CTxMemPoolEntry header only…

hebasto commented at 7:47 am on January 24, 2020: member

@JeremyRubin

It’s annoying, but because these functions were previously in the same translation unit then it’s possible they can get completely inlined, but now they’re in separate units they will definitely trigger function calls.

IIUC, making function inline depends on compiler and its optimization settings. The bitcoind compiled with GCC 7.4.0 was checked with nm tool, and CTxMemPoolEntry member function symbols are present, therefore those functions are not inlined (except those which are defined in class declaration, as expected).

It seems runtime performance issues and circular dependency resolving are orthogonal. And “yes”, we could

… make CTxMemPoolEntry header only…

JeremyRubin commented at 6:27 pm on January 24, 2020: contributor

Correct; this is compiler dependent behavior. But we happen to compile with optimizations enabled for release builds – not sure if we do different flags for non release builds (I think it’s the same?).

Presence of the symbols doesn’t prove that they aren’t also inlined. An inline function is a hint to inline, not a mandate, and the function can be either called or or inlined depending on context, you would need to check the actual call sites.

I agree it’s not a large issue; but specifically code organization does affect emitted code (this is what LTO is intended, in a sense, to get around, but I don’t think we do that other than experimentally)

hebasto commented at 6:35 pm on January 24, 2020: member

Presence of the symbols doesn’t prove that they aren’t also inlined. An inline function is a hint to inline, not a mandate, and the function can be either called or or inlined depending on context, you would need to check the actual call sites.

I mean if a function is inlined in all call sites, there is no dedicated assembly code and no symbol. Could a compiler inline a function in some call sites and make a standard function call of the same function in other call sites?

gmaxwell commented at 6:54 pm on January 24, 2020: contributor

Unless the symbol isn’t exported there will be a separate copy of it included in the object for external callers. That extra copy will persist in the final binary so long as there are any non-inlined callers.

For a quick check, just look at the size of all functions in dump of the binary. If they’re all the same size then it’s very likely that all you did was move things around.

sipa commented at 7:19 pm on January 24, 2020: member

In C++, inline is more than a hint, and actually has a meaning: it’s a function that may be defined (and exported from) multiple translation units without conflicting with itself (it’s only well defined if all definitions are identical, though). This is what allows having code in .h files. In practice it means the linker will merge all the definitions into one symbol.

JeremyRubin commented at 11:20 pm on January 24, 2020: contributor

I think what @gmaxwell says is correct with my question. @sipa that sounds correct. These weren’t previously marked inline either, so I guess what I meant to capture is that not marking inline is a hint not to inline but not a mandate to not inline. E.g., if some function F is called by G, F may or may not be inlined depending on the compiler if they are in the same translation unit, but certainly won’t be if F is in a different translation unit, is non inline, and LTO is not enabled.

JeremyRubin commented at 9:47 pm on January 28, 2020: contributor

I think if you were to just put all the mempool entry functions into the header I wouldn’t have any concerns. It doesn’t seem to me like there’s a really strong reason for them to be separate, and any time you change the initializers you’ve likely also changed the class definition so it doesn’t seem like it would trigger recompilation explosions.

It also (maybe I’m wrong here) looks like you have some unneeded dependencies in the cpp (e.g., memusage), so that can be pruned out to keep the header small.

hebasto force-pushed on Jan 29, 2020

hebasto commented at 8:26 pm on January 29, 2020: member

@JeremyRubin Thank you for your review.

I think if you were to just put all the mempool entry functions into the header I wouldn’t have any concerns.

Done.

It also (maybe I’m wrong here) looks like you have some unneeded dependencies in the cpp (e.g., memusage), so that can be pruned out to keep the header small.

All #includes are checked again. They all are needed, e.g., core_memusage.h is needed for RecursiveDynamicUsage.

DrahtBot added the label Needs rebase on Jan 30, 2020

hebasto force-pushed on Jan 30, 2020

hebasto commented at 4:53 pm on January 30, 2020: member

Rebased after #17261 has been merged.

DrahtBot removed the label Needs rebase on Jan 30, 2020

maflcko commented at 7:38 pm on January 30, 2020: member

Looking at the (currently) 9 conflicts, maybe it makes sense to postpone this changeset until after some of the other user-facing/features got merged to the mempool code?

DrahtBot added the label Needs rebase on Feb 3, 2020

hebasto force-pushed on Feb 3, 2020

hebasto commented at 7:32 pm on February 3, 2020: member

Rebased after #17925 has been merged.

DrahtBot removed the label Needs rebase on Feb 3, 2020

Empact commented at 8:51 pm on February 20, 2020: member

ACK https://github.com/bitcoin/bitcoin/pull/17786/commits/19f6bf826d890cc78bd4a19f10dfbf59dd58c10d

A few possible refinements: https://github.com/hebasto/bitcoin/compare/20191221-mempool-circ-dep...Empact:2020-02-mempool-circ-dep

hebasto commented at 4:54 pm on April 11, 2020: member

Updated 19f6bf826d890cc78bd4a19f10dfbf59dd58c10d -> 287b930a18921716bdf6961350959804042b46b0 (pr17786.07 -> pr17786.08, diff):

implemented @Empact’s suggestions

Empact commented at 9:20 pm on April 11, 2020: member

Re-ACK https://github.com/bitcoin/bitcoin/pull/17786/commits/287b930a18921716bdf6961350959804042b46b0 - reviewed diff particularly changed lines as identified by git. Didn’t audit the includes.

DrahtBot added the label Needs rebase on Jul 1, 2020

hebasto force-pushed on Jul 3, 2020

hebasto commented at 7:01 am on July 3, 2020: member

Rebased 287b930a18921716bdf6961350959804042b46b0 -> 219324ca4fe0d466c9007c30543c88444dec3569 (pr17786.08 -> pr17786.09) due to the conflict with #19331.

DrahtBot removed the label Needs rebase on Jul 3, 2020

DrahtBot added the label Needs rebase on Sep 7, 2020

hebasto force-pushed on Sep 7, 2020

hebasto commented at 12:14 pm on September 7, 2020: member

Rebased 219324ca4fe0d466c9007c30543c88444dec3569 -> 9782a8ee773a59667424353de047fd6bb9c3cb5d (pr17786.09 -> pr17786.10) due to the conflict with #19478.

DrahtBot removed the label Needs rebase on Sep 7, 2020

in src/txmempool_entry.h:90 in 9782a8ee77 outdated

76+    LockPoints lockPoints;          //!< Track the height and time at which tx was final
77+
78+    // Information about descendants of this transaction that are in the
79+    // mempool; if we remove this transaction we must remove all of these
80+    // descendants as well.
81+    uint64_t nCountWithDescendants{1}; //!< number of descendant transactions

JeremyRubin commented at 6:26 pm on September 7, 2020:

nit: slight preference to do all initialization in the constructor (for consistency/minimal diff)

JeremyRubin approved

JeremyRubin commented at 6:27 pm on September 7, 2020: contributor

Looks correct / refactor only.

CR-Ack.

hebasto commented at 6:30 pm on September 7, 2020: member

@promag @empact Mind re-reviewing?

Empact commented at 11:36 pm on September 7, 2020: member

Code Review ACK https://github.com/bitcoin/bitcoin/pull/17786/commits/9782a8ee773a59667424353de047fd6bb9c3cb5d

DrahtBot added the label Needs rebase on Dec 1, 2020

hebasto closed this on Aug 24, 2021

maflcko commented at 2:45 pm on September 21, 2021: member

Why the close?

maflcko added the label Up for grabs on Sep 23, 2021

hebasto reopened this on Sep 24, 2021

hebasto force-pushed on Sep 24, 2021

hebasto commented at 6:44 pm on September 24, 2021: member

@MarcoFalke

Why the close?

Reopened and rebased 9782a8ee773a59667424353de047fd6bb9c3cb5d -> 77438e18478ca0a06708f3e988fdb31567bb76e0 (pr17786.10 -> pr17786.11) on top of the merged #23054 and the recent CI changes.

hebasto removed the label Up for grabs on Sep 24, 2021

DrahtBot removed the label Needs rebase on Sep 24, 2021

in src/txmempool_entry.h:120 in 9161fefe06 outdated

112+    const CTransaction& GetTx() const { return *this->tx; }
113+    CTransactionRef GetSharedTx() const { return this->tx; }
114+    const CAmount& GetFee() const { return nFee; }
115+    size_t GetTxSize() const
116+    {
117+        return GetVirtualTransactionSize(nTxWeight, sigOpCost);

maflcko commented at 7:49 am on September 25, 2021:

I think it makes sense to keep this in the cpp file, to keep the header free of unneeded includes.

hebasto commented at 7:53 am on September 25, 2021:

There were concerns from @JeremyRubin:

I think if you were to just put all the mempool entry functions into the header I wouldn’t have any concerns.

DrahtBot added the label Needs rebase on Oct 25, 2021

hebasto force-pushed on Dec 4, 2021

hebasto commented at 7:52 pm on December 4, 2021: member

Rebased 77438e18478ca0a06708f3e988fdb31567bb76e0 -> 785968d88833c1fb4018050849f88b6014a4e8eb (pr17786.11 -> pr17786.12) on top of the recent conflicting changes.

DrahtBot removed the label Needs rebase on Dec 4, 2021

DrahtBot added the label Needs rebase on Dec 8, 2021

hebasto force-pushed on Dec 29, 2021

hebasto commented at 8:25 pm on December 29, 2021: member

Rebased 785968d88833c1fb4018050849f88b6014a4e8eb -> 85a7dcb799e8f0d10f8b58d23a32aeb14ea872ef (pr17786.12 -> pr17786.13) on top of the recent conflicting changes.

DrahtBot removed the label Needs rebase on Dec 29, 2021

DrahtBot added the label Needs rebase on Jan 2, 2022

hebasto force-pushed on Jan 2, 2022

hebasto commented at 10:31 am on January 2, 2022: member

Rebased 85a7dcb799e8f0d10f8b58d23a32aeb14ea872ef -> b0225a6f26876873265cc76e2ad8f68e35401bdf (pr17786.13 -> pr17786.14) due to the conflict with #23795.

DrahtBot removed the label Needs rebase on Jan 2, 2022

glozow commented at 5:20 pm on January 4, 2022: member

Concept ACK to removing this circular dependency

But perhaps a better approach would be to remove minerPolicyEstimator from txmempool, make it a client of CValidationInterface instead (will need to return more information to clients in the interface), and remove policy/fees as a dependency of txmempool. In fact, it seems like background fee estimator has been planned for a while (https://github.com/bitcoin/bitcoin/pull/10199#discussion_r115530273) and implemented but unmerged at some point (#11775).

DrahtBot added the label Needs rebase on Jan 25, 2022

hebasto force-pushed on Jan 26, 2022

hebasto commented at 9:17 am on January 26, 2022: member

Rebased b0225a6f26876873265cc76e2ad8f68e35401bdf -> 122db971c4e1283d8e500b03edbc0b026ad23d5a (pr17786.14 -> pr17786.15) due to the conflict with #21464.

DrahtBot removed the label Needs rebase on Jan 26, 2022

DrahtBot added the label Needs rebase on Feb 1, 2022

hebasto force-pushed on Feb 2, 2022

hebasto commented at 3:57 pm on February 2, 2022: member

Rebased 122db971c4e1283d8e500b03edbc0b026ad23d5a -> f7297485cf2388352bd36bd2d4dea6ba009bba21 (pr17786.15 -> pr17786.16) due to the conflict with #24218.

DrahtBot removed the label Needs rebase on Feb 2, 2022

DrahtBot added the label Needs rebase on Feb 25, 2022

hebasto force-pushed on Apr 5, 2022

hebasto commented at 6:59 pm on April 5, 2022: member

Rebased f7297485cf2388352bd36bd2d4dea6ba009bba21 -> 8675c9ae35aa37428732ed91c04dae94183e0705 (pr17786.16 -> pr17786.17).

DrahtBot removed the label Needs rebase on Apr 5, 2022

DrahtBot added the label Needs rebase on Apr 25, 2022

hebasto force-pushed on May 28, 2022

hebasto commented at 11:37 am on May 28, 2022: member

Rebased 8675c9ae35aa37428732ed91c04dae94183e0705 -> 083b8166e032dc6ea20eed4d13d3353e90a0a633 (pr17786.17 -> pr17786.18) due to the conflict with #24915.

DrahtBot removed the label Needs rebase on May 28, 2022

DrahtBot added the label Needs rebase on Jun 7, 2022

hebasto force-pushed on Jun 12, 2022

hebasto commented at 12:25 pm on June 12, 2022: member

Rebased 083b8166e032dc6ea20eed4d13d3353e90a0a633 -> 4a107ebe5892272cfb4b74085b2ab4bb06500b72 (pr17786.18 -> pr17786.19) due to the conflict with #25254.

DrahtBot removed the label Needs rebase on Jun 12, 2022

DrahtBot added the label Needs rebase on Jun 27, 2022

hebasto force-pushed on Jul 19, 2022

hebasto commented at 8:03 pm on July 19, 2022: member

Rebased 4a107ebe5892272cfb4b74085b2ab4bb06500b72 -> 3a430a4700780173cf4ea98ec9b4dbc2fc7b1984 (pr17786.19 -> pr17786.20).

DrahtBot removed the label Needs rebase on Jul 19, 2022

DrahtBot added the label Needs rebase on Aug 3, 2022

hebasto force-pushed on Aug 3, 2022

hebasto commented at 2:00 pm on August 3, 2022: member

Rebased 3a430a4700780173cf4ea98ec9b4dbc2fc7b1984 -> c4c311ea83338709a7b66c49a2364a65ae430c50 (pr17786.20 -> pr17786.21) due to the conflict with #25648.

DrahtBot removed the label Needs rebase on Aug 3, 2022

DrahtBot added the label Needs rebase on Oct 5, 2022

hebasto force-pushed on Oct 5, 2022

hebasto commented at 11:28 am on October 5, 2022: member

Rebased c4c311ea83338709a7b66c49a2364a65ae430c50 -> 11d885bc3dd9a44af4c7007a95b4fbb6076e4428 (pr17786.21 -> pr17786.22) due to the conflict with #26250.

DrahtBot removed the label Needs rebase on Oct 5, 2022

DrahtBot added the label Needs rebase on Oct 13, 2022

hebasto force-pushed on Oct 14, 2022

hebasto commented at 10:55 am on October 14, 2022: member

Rebased 11d885bc3dd9a44af4c7007a95b4fbb6076e4428 -> f140f9468bfa505a34112d625128f3fcff18ddba (pr17786.22 -> pr17786.23) due to the conflict with #25667.

DrahtBot removed the label Needs rebase on Oct 14, 2022

ryanofsky approved

ryanofsky commented at 6:32 pm on November 16, 2022: contributor

Code review ACK f140f9468bfa505a34112d625128f3fcff18ddba. This seems like an improvement over status quo to remove a circular dependency and improve code organization.

I agree with Gloria in #17786 (comment) other improvements could also be made which remove the circular dependency, but they seem complimentary and could build on this.

I think all the other discussion about code inlining and runtime performance and build performance makes sense, but I don’t think those issues are a lot more pertinent to this PR than a lot of other PRs. So it just seems unlucky for this PR that they were brought up here. It would be good if we could think of a standard approach to avoiding performance regressions that could apply more fairly.

One other suggestion to make the PR more reviewable might be to split it into two commits: a move-only commit that just moved the CTxMemPoolEntry class definition without changing it, followed by a commit inlining its various methods. (This PR has been open a while and I probably would have reviewed it earlier if the bigger moveonly change didn’t include other changes and was easier to verify.)

hebasto force-pushed on Nov 16, 2022

hebasto commented at 8:13 pm on November 16, 2022: member

@ryanofsky

Thank you for your review!

One other suggestion to make the PR more reviewable might be to split it into two commits: a move-only commit that just moved the CTxMemPoolEntry class definition without changing it, followed by a commit inlining its various methods.

Done.

refactor: Move `CTxMemPoolEntry` class to its own module

This change nukes the policy/fees->mempool circular dependency.

Easy to review using `diff --color-moved=dimmed-zebra`.

75bbe594e5

refactor: Inline `CTxMemPoolEntry` class's functions c8dc0e3eaa

hebasto force-pushed on Nov 16, 2022

ryanofsky approved

ryanofsky commented at 4:18 pm on November 17, 2022: contributor

Code review ACK c8dc0e3eaa9e0f956c5177bcb69632beb0d51770. Just include and whitespace changes since last review, and there’s a moveonly commit now so it’s very easy to review

theStack approved

theStack commented at 5:55 pm on November 17, 2022: contributor

Code-review ACK c8dc0e3eaa9e0f956c5177bcb69632beb0d51770

Nice to see another circular dependency gone 💯

maflcko removed the label Refactoring on Nov 17, 2022

DrahtBot added the label Refactoring on Nov 17, 2022

fanquake requested review from glozow on Nov 18, 2022

glozow commented at 0:40 am on November 19, 2022: member

utACK c8dc0e3eaa9e0f956c5177bcb69632beb0d51770, agree these changes are an improvement.

glozow merged this on Nov 19, 2022

glozow closed this on Nov 19, 2022

hebasto deleted the branch on Nov 19, 2022

JeremyRubin commented at 9:51 pm on November 19, 2022: contributor

the reason it was brought up historically is that circa 2019/early 2020 I thought we might be able to increase mempool limits, and these functions end up being relatively hot, especially with larger workloads, so it made sense to be careful with them. @jamesob would probably be the guy to ask about tracking perf regressions….

sidhujag referenced this in commit 5c18092e84 on Nov 20, 2022

in src/Makefile.am:264 in c8dc0e3eaa

260@@ -261,6 +261,7 @@ BITCOIN_CORE_H = \
261   torcontrol.h \
262   txdb.h \
263   txmempool.h \
264+  txmempool_entry.h \

maflcko commented at 10:13 am on November 21, 2022:

If this is moved, why not move it to the right place, that is to kernel/txmempool_entry.h?

hebasto commented at 10:42 am on November 30, 2022:

@MarcoFalke #26609.

in test/sanitizer_suppressions/ubsan:66 in c8dc0e3eaa

62@@ -62,6 +63,7 @@ implicit-integer-sign-change:script/bitcoinconsensus.cpp
63 implicit-integer-sign-change:script/interpreter.cpp
64 implicit-integer-sign-change:serialize.h
65 implicit-integer-sign-change:txmempool.cpp
66+implicit-integer-sign-change:txmempool_entry.h

maflcko commented at 10:13 am on November 21, 2022:

Are they still needed?

hebasto commented at 11:02 am on November 21, 2022:

Apparently, they do not. Removed in needless ~#26542~ #26545.

Thank you!

maflcko referenced this in commit 1ff79292e3 on Dec 6, 2022

bitcoin locked this on Nov 30, 2023

refactor: Nuke policy/fees->mempool circular dependencies #17786

Reviews

Conflicts