Implement accurate memory accounting for mempool #6410

Merged
merged 1 commit into from Jul 11, 2015

Conversation

Projects
None yet
6 participants
@sipa
Member

sipa commented Jul 9, 2015

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 (#6281).

@sipa sipa referenced this pull request Jul 9, 2015

Closed

Limit mempool size #6281

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jul 10, 2015

Member

Tested ACK.

Nit: I'd slightly prefer DynamicUsage for eg uint256 as helper functions outside the class instead of added members.

Member

laanwj commented Jul 10, 2015

Tested ACK.

Nit: I'd slightly prefer DynamicUsage for eg uint256 as helper functions outside the class instead of added members.

@laanwj laanwj added the RPC/REST/ZMQ label Jul 10, 2015

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jul 10, 2015

Member

Updated to retain the totaltxsize field in the RPC output.

@laanwj I avoided the need for a DynamicMemoryUsage for uint256 altogether now.

Member

sipa commented Jul 10, 2015

Updated to retain the totaltxsize field in the RPC output.

@laanwj I avoided the need for a DynamicMemoryUsage for uint256 altogether now.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jul 10, 2015

Member

Ok, even better.

Member

laanwj commented Jul 10, 2015

Ok, even better.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jul 11, 2015

Member

Review ACK.
utACK (code is now running on bitcoin.jonasschnelli.ch)
very minor nit: some ifs have opening brackets on the same line, some not. But i don't care.

Member

jonasschnelli commented Jul 11, 2015

Review ACK.
utACK (code is now running on bitcoin.jonasschnelli.ch)
very minor nit: some ifs have opening brackets on the same line, some not. But i don't care.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jul 11, 2015

Member
Member

sipa commented Jul 11, 2015

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Jul 11, 2015

Member

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...

Member

jtimon commented Jul 11, 2015

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...

@laanwj laanwj merged commit 5098c47 into bitcoin:master Jul 11, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jul 11, 2015

Merge pull request #6410
5098c47 Implement accurate memory accounting for mempool (Pieter Wuille)
@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jul 11, 2015

Member

Here are also some stats done with a node running this PR: http://bitcoin.jonasschnelli.ch/charts/mempool/

Member

jonasschnelli commented Jul 11, 2015

Here are also some stats done with a node running this PR: http://bitcoin.jonasschnelli.ch/charts/mempool/

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Jul 11, 2015

Contributor

utACK

Contributor

petertodd commented Jul 11, 2015

utACK

@@ -58,6 +59,7 @@ class CTxMemPoolEntry
int64_t GetTime() const { return nTime; }
unsigned int GetHeight() const { return nHeight; }
bool WasClearAtEntry() const { return hadNoDependencies; }
+ size_t DynamicMemoryUsage() const { return nUsageSize; }

This comment has been minimized.

@morcos

morcos Jul 13, 2015

Member

Any reason we're not counting the other member variables? Seems like there are another 53 bytes per CTMemPoolEntry?

@morcos

morcos Jul 13, 2015

Member

Any reason we're not counting the other member variables? Seems like there are another 53 bytes per CTMemPoolEntry?

This comment has been minimized.

@sipa

sipa Jul 13, 2015

Member

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.

@sipa

sipa Jul 13, 2015

Member

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.

@str4d str4d referenced this pull request in zcash/zcash Mar 14, 2017

Merged

Bitcoin 0.12 mempool memory usage PRs #2175

zkbot added a commit to zcash/zcash that referenced this pull request Mar 23, 2017

Auto merge of #2175 - str4d:2074-txn-mempool, r=bitcartel
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment