-
Notifications
You must be signed in to change notification settings - Fork 38.8k
Implement accurate memory accounting for mempool #6410
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
45f50ac to
38ac0b5
Compare
|
Tested ACK. Nit: I'd slightly prefer |
|
Updated to retain the totaltxsize field in the RPC output. @laanwj I avoided the need for a DynamicMemoryUsage for uint256 altogether now. |
|
Ok, even better. |
|
Review ACK. |
|
Re code style: I'm aware, but I'm always following the style of the
surroundig code.
|
|
utACK Regarding style, I believe https://github.com/bitcoin/bitcoin/blob/master/src/.clang-format says always in the same line for if, else, for, switch and always in the next line for functions/methods (unless they are defined in one line, which is allowed). That's what I'm doing when I need to touch the lines, but I have to admit I'm not 100% sure. Anyway, style nits... |
5098c47 Implement accurate memory accounting for mempool (Pieter Wuille)
|
Here are also some stats done with a node running this PR: http://bitcoin.jonasschnelli.ch/charts/mempool/ |
|
utACK |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason we're not counting the other member variables? Seems like there are another 53 bytes per CTMemPoolEntry?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it should have been called IndirectMemoryUsage. The size of the CTxMemPoolEntry structure itself is accounted for through memusage::DynamicUsage(mapTx), in which the entries are included. That allows for accurately taking alignment and malloc overhead into account.
Bitcoin 0.12 mempool memory usage PRs Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#6410 - bitcoin/bitcoin#6453 - bitcoin/bitcoin#6013 (excludes changes to docs we deleted) Part of #2074.
368665e Implement accurate memory accounting for mempool (random-zebra) Pull request description: Based on top of: - [x] #1641 This continues the work of #1531 adding memory accounting for the mempool. Backports bitcoin#6410. Original description: > This implements accurate memory usage accounting for the mempool. It is only exposed through getmempoolinfo for now, but could be used for limiting the resource requirements too (bitcoin#6281). ACKs for top commit: furszy: pretty nice, tested ACK 368665e Fuzzbawls: ACK 368665e Tree-SHA512: f1dd0e98af58133255db02ae57f20c5d1c0b210610bf6e6c99a112c8c74c0e83e0ae05fd22a933cc4db0eaca36b5f45fa27231879809b348ba0dba034e176767
368665e35981b28c8d2f0c982ea493996358bb05 Implement accurate memory accounting for mempool (random-zebra) Pull request description: Based on top of: - [x] #1641 This continues the work of #1531 adding memory accounting for the mempool. Backports bitcoin/bitcoin#6410. Original description: > This implements accurate memory usage accounting for the mempool. It is only exposed through getmempoolinfo for now, but could be used for limiting the resource requirements too (bitcoin/bitcoin#6281). ACKs for top commit: furszy: pretty nice, tested ACK 368665e Fuzzbawls: ACK 368665e35981b28c8d2f0c982ea493996358bb05 Tree-SHA512: f1dd0e98af58133255db02ae57f20c5d1c0b210610bf6e6c99a112c8c74c0e83e0ae05fd22a933cc4db0eaca36b5f45fa27231879809b348ba0dba034e176767 # Conflicts: # src/memusage.h # src/script/script.cpp # src/script/script.h
This implements accurate memory usage accounting for the mempool. It is only exposed through
getmempoolinfofor now, but could be used for limiting the resource requirements too (#6281).