Skip to content

Accurately account for mempool index memory #26614

Closed
hebasto wants to merge 9 commits intobitcoin:masterfrom
hebasto:221130-account
Closed

Accurately account for mempool index memory #26614
hebasto wants to merge 9 commits intobitcoin:masterfrom
hebasto:221130-account

Conversation

@hebasto
Copy link
Member

@hebasto hebasto commented Nov 30, 2022

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.

@DrahtBot
Copy link
Contributor

DrahtBot commented Nov 30, 2022 •
edited
Loading

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 hebasto mentioned this pull request Nov 30, 2022
Closed
@hebasto hebasto mentioned this pull request Nov 30, 2022
Closed
This was referenced Dec 1, 2022
Merged
Merged
@hebasto hebasto marked this pull request as ready for review December 1, 2022 20:04
@fanquake
Copy link
Member

fanquake commented Dec 20, 2022

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
Copy link
Contributor

DrahtBot commented Jan 4, 2023

🐙 This pull request conflicts with the target branch and needs rebase.

@hebasto hebasto closed this Mar 21, 2023
@martinus martinus mentioned this pull request May 25, 2023
Closed
@bitcoin bitcoin locked and limited conversation to collaborators Mar 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

Mempool Needs rebase Resource usage

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@hebasto @DrahtBot @fanquake @sipa