Accurately account for mempool index memory #26614

pull hebasto wants to merge 9 commits into bitcoin:master from hebasto:221130-account changing 7 files +204 −21

hebasto commented at 10:49 pm on November 30, 2022: member

Picked #18086:

This introduces a C++ allocator class (memusage::AccountingAllocator) which enables containers that accurately account for all their memory allocations in a tracker variable.

This is then used to replace the heuristics in the mempool to guess memory usage.
test: Make `mempool_tests/MempoolSizeLimitTest` allocation-neutral 90e4fc7faf
hebasto added the label Mempool on Nov 30, 2022
hebasto added the label Resource usage on Nov 30, 2022
DrahtBot commented at 10:49 pm on November 30, 2022: 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. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:
- #25325 (Add pool based memory resource by martinus)
- #19909 (refactor: Remove unused CTxMemPool::clear() helper by MarcoFalke)
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.
hebasto force-pushed on Nov 30, 2022
hebasto force-pushed on Dec 1, 2022
Add `AccountingAllocator` to help with memusage tracking
Co-authored-by: Pieter Wuille <pieter.wuille@gmail.com>
3d585bd71d

test: Add `AccountingAllocator` tests

Co-authored-by: Pieter Wuille <pieter.wuille@gmail.com>

5e2f44f957

Account `CTxMemPool::mapTx` accurately 28f2c42e0f
Account `CTxMemPool::mapDeltas` accurately a66e9114b7
Account `CTxMemPool::vTxHashes` accurately 141cbe3755
Add allocator support to `indirectmap` 8cc3b95b2e
Account `CTxMemPool::mapNextTx` accurately b267080105
Account `DisconnectedBlockTransactions::queuedTx` accurately 64b0d3c230
hebasto force-pushed on Dec 1, 2022
hebasto marked this as ready for review on Dec 1, 2022
fanquake commented at 10:55 am on December 20, 2022: member
It would be good for the PR description to outline:
- What is currently wrong with our memory accounting?
  
  How inaccurate is it?
- How (much more) accurate is it after this change?
  
  Is the accuracy affected by things like standard library (version), compiler used etc.
- How does someone reviewing this PR check the accounted memory usage, before and after?
Maybe the above is all completely obvious from the changes, but I think a PR like this would benefit from having a PR description that isn’t just copy-paste from the original 2-year-old PR. Especially since this PR is also missing all the discussion/questions that happened in the original PR (anything relevant could be recapped in the description here).
DrahtBot added the label Needs rebase on Jan 4, 2023
DrahtBot commented at 8:52 am on January 4, 2023: contributor

🐙 This pull request conflicts with the target branch and needs rebase.
hebasto closed this on Mar 21, 2023
bitcoin locked this on Mar 20, 2024