refactor: Nuke policy/fees->mempool circular dependencies #17786
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
Concept ACK. |
|
Concept ACK: thanks for nuking circular dependencies |
fbf6b75 to
045794b
Compare
Done. |
|
|
f2cb595 to
1e540cd
Compare
|
Travis is happy now ;) |
|
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:
|
How is it possible? |
|
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... |
IIUC, making function inline depends on compiler and its optimization settings. The It seems runtime performance issues and circular dependency resolving are orthogonal. And "yes", we could
|
|
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) |
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? |
|
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. |
|
In C++, |
|
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. |
|
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. |
1e540cd to
13a4a68
Compare
|
@JeremyRubin Thank you for your review.
Done.
All |
13a4a68 to
ac83af8
Compare
|
Rebased after #17261 has been merged. |
|
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? |
11d885b to
f140f94
Compare
|
Rebased 11d885b -> f140f94 (pr17786.22 -> pr17786.23) due to the conflict with #25667. |
There was a problem hiding this comment.
Code review ACK f140f94. 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.)
f140f94 to
fed9e8e
Compare
|
Thank you for your review!
Done. |
This change nukes the policy/fees->mempool circular dependency. Easy to review using `diff --color-moved=dimmed-zebra`.
fed9e8e to
c8dc0e3
Compare
|
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.... |
| @@ -261,6 +261,7 @@ BITCOIN_CORE_H = \ | |||
| torcontrol.h \ | |||
| txdb.h \ | |||
| txmempool.h \ | |||
| txmempool_entry.h \ | |||
There was a problem hiding this comment.
If this is moved, why not move it to the right place, that is to kernel/txmempool_entry.h?
| @@ -62,6 +63,7 @@ implicit-integer-sign-change:script/bitcoinconsensus.cpp | |||
| implicit-integer-sign-change:script/interpreter.cpp | |||
| implicit-integer-sign-change:serialize.h | |||
| implicit-integer-sign-change:txmempool.cpp | |||
| implicit-integer-sign-change:txmempool_entry.h | |||
…kernel/mempool_entry.h` 38941a7 refactor: Move `txmempool_entry.h` --> `kernel/mempool_entry.h` (Hennadii Stepanov) Pull request description: This PR addresses the bitcoin/bitcoin#17786 (comment): > why not move it to the right place, that is to `kernel/txmempool_entry.h`? ACKs for top commit: MarcoFalke: review ACK 38941a7 📊 Tree-SHA512: 0145974b63b67ca1d9d89af2dd9d4438beca480c16a563f330da05fec49b8394d7ba20ed83cf7d50b2e19454e006978ebed42b0e07887b98d00210f3201ce9ba
This PR:
policy/fees->txmempool->policy/feescircular dependency