Add DynamicMemoryUsage() to CDBWrapper to estimate LevelDB memory use #12604

eklitzke · 2018-03-05T19:41:27Z

This adds a new method CDBWrapper::DynamicMemoryUsage() similar to Bitcoin's existing methods of the same name. It's implemented by asking LevelDB for the information, and then parsing the string response. I've also added logging to CDBWrapper::WriteBatch() to track this information:

$ tail -f ~/.bitcoin/testnet3/debug.log | grep WriteBatch
2018-03-05 19:34:55 WriteBatch memory usage: db=chainstate, before=0.0MiB, after=0.0MiB
2018-03-05 19:35:17 WriteBatch memory usage: db=index, before=0.0MiB, after=0.0MiB
2018-03-05 19:35:17 WriteBatch memory usage: db=chainstate, before=0.0MiB, after=8.0MiB
2018-03-05 19:35:22 WriteBatch memory usage: db=index, before=0.0MiB, after=0.0MiB
2018-03-05 19:35:22 WriteBatch memory usage: db=chainstate, before=8.0MiB, after=17.0MiB
2018-03-05 19:35:26 WriteBatch memory usage: db=index, before=0.0MiB, after=0.0MiB
2018-03-05 19:35:27 WriteBatch memory usage: db=chainstate, before=9.0MiB, after=18.0MiB
2018-03-05 19:35:40 WriteBatch memory usage: db=index, before=0.0MiB, after=0.0MiB
2018-03-05 19:35:41 WriteBatch memory usage: db=chainstate, before=9.0MiB, after=7.0MiB
2018-03-05 19:35:52 WriteBatch memory usage: db=index, before=0.0MiB, after=0.0MiB
2018-03-05 19:35:52 WriteBatch memory usage: db=chainstate, before=7.0MiB, after=9.0MiB
^C

As LevelDB doesn't seem to provide a way to get the database name, I've also added a new m_name field to the CDBWrapper. This is necessary because we have multiple LevelDB databases (two now, and possibly more later, e.g. #11857).

I am using this information in other branches where I'm experimenting with changing LevelDB buffer sizes.

laanwj · 2018-03-05T20:23:58Z

src/dbwrapper.cpp

We might want to skip this call (and the one after) if BCLog::LEVELDB is disabled, with the string parsing and floating point arithmetic it seems a lot of overhead for a log message that will be ignored anyway.

Good observation, fixed.

promag · 2018-03-05T20:30:57Z

src/dbwrapper.cpp

Use http://en.cppreference.com/w/cpp/string/basic_string/stoul instead.

promag · 2018-03-05T20:32:05Z

src/dbwrapper.h

Could be const?

Maybe it's not possible, since GetProperty is not const

Right, unless you want to give into the temptation of const cast.

Nah, IMO it's fine as it is.

Maybe it's not possible, since GetProperty is not const

FWIW, it is possible to declare this const, because GetProperty is called on a pointer to a non-const object. Adding const here only makes the pointer address const, not the object located at that address.

promag · 2018-03-05T20:32:19Z

src/dbwrapper.cpp

Nit, space after :.

BTW, could use path in the debug instead?

relevant considering External wallet files #11687

CDBWrapper constructor also logs path.

Added space. I kind of like the short path in the logs because it makes the lines more readable, what if I did the following:

Add new name parameter to constructor

Chainstate/blocks databases will pass in "chainstate" "blocks" etc. to explicitly name the object

If an explicit name is omitted the path will be logged, and this pattern could be used in External wallet files #11687 since wallet files are not global like the other databases.

promag · 2018-03-05T20:33:38Z

src/dbwrapper.cpp

Nit, initialize = 0.

done (although I think it's fine to keep unused variables uninitialized)

promag · 2018-03-05T20:34:14Z

src/dbwrapper.cpp

Nit, same line.

I added braces, which I think is what you mean here?

promag · 2018-03-05T20:34:55Z

src/dbwrapper.cpp

Nit

if (log_memory) { double mem_after = ...; LogPrint(BCLog::LEVELDB, ...); }

promag · 2018-03-05T20:39:12Z

src/dbwrapper.cpp

Suggestion:

if (!pdb->GetProperty(...)) { LogPrint(BCLog::LEVELDB, "Failed to get approximate-memory-usage property\n"); return 0; } return stoul(memory);

sipa · 2018-03-05T20:55:04Z

src/dbwrapper.cpp

Nit: braces whenever the then branch is not on the same line.

promag · 2018-03-05T21:13:43Z

utACK after squash.

laanwj · 2018-03-05T21:17:48Z

utACK

ryanofsky

utACK e7fa704cbaa5de2ca9bfffe560ff6c6fdf751ac0

ryanofsky · 2018-03-05T22:22:59Z

src/dbwrapper.h

Maybe it's not possible, since GetProperty is not const

FWIW, it is possible to declare this const, because GetProperty is called on a pointer to a non-const object. Adding const here only makes the pointer address const, not the object located at that address.

This adds a DynamicMemoryUsage() method similar to the existing methods of the same name, and adds logging of memory usage to CDBWrapper::WriteBatch.

eklitzke · 2018-03-06T05:27:24Z

Updated with 741f017 to fix const correctness.

jonasschnelli · 2018-03-06T05:44:51Z

Nice!
utACK 741f017

practicalswift · 2018-03-06T06:13:28Z

utACK 741f017

ryanofsky

utACK 741f017. Differences since last review were various style cleanups that don't change behavior.

…lDB memory use 741f017 Add DynamicMemoryUsage() to LevelDB (Evan Klitzke) Pull request description: This adds a new method `CDBWrapper::DynamicMemoryUsage()` similar to Bitcoin's existing methods of the same name. It's implemented by asking LevelDB for the information, and then parsing the string response. I've also added logging to `CDBWrapper::WriteBatch()` to track this information: ``` $ tail -f ~/.bitcoin/testnet3/debug.log | grep WriteBatch 2018-03-05 19:34:55 WriteBatch memory usage: db=chainstate, before=0.0MiB, after=0.0MiB 2018-03-05 19:35:17 WriteBatch memory usage: db=index, before=0.0MiB, after=0.0MiB 2018-03-05 19:35:17 WriteBatch memory usage: db=chainstate, before=0.0MiB, after=8.0MiB 2018-03-05 19:35:22 WriteBatch memory usage: db=index, before=0.0MiB, after=0.0MiB 2018-03-05 19:35:22 WriteBatch memory usage: db=chainstate, before=8.0MiB, after=17.0MiB 2018-03-05 19:35:26 WriteBatch memory usage: db=index, before=0.0MiB, after=0.0MiB 2018-03-05 19:35:27 WriteBatch memory usage: db=chainstate, before=9.0MiB, after=18.0MiB 2018-03-05 19:35:40 WriteBatch memory usage: db=index, before=0.0MiB, after=0.0MiB 2018-03-05 19:35:41 WriteBatch memory usage: db=chainstate, before=9.0MiB, after=7.0MiB 2018-03-05 19:35:52 WriteBatch memory usage: db=index, before=0.0MiB, after=0.0MiB 2018-03-05 19:35:52 WriteBatch memory usage: db=chainstate, before=7.0MiB, after=9.0MiB ^C ``` As LevelDB doesn't seem to provide a way to get the database name, I've also added a new `m_name` field to the `CDBWrapper`. This is necessary because we have multiple LevelDB databases (two now, and possibly more later, e.g. #11857). I am using this information in other branches where I'm experimenting with changing LevelDB buffer sizes. Tree-SHA512: 7ea8ff5484bb07ef806af17d000c74ccca27d2e0f6c3229e12d93818f00874553335d87428482bd8acbcae81ea35aef2a243326f9fccbfac25989323d24391b4

Summary: Backport of Bitcoin Core PR12604 and PR12784 bitcoin/bitcoin#12604 bitcoin/bitcoin#12784 Test Plan: ``` make check ``` Run `bitcoind` with `-debug=leveldb` and check the log file for the `LEVELDB` category and the following pattern: `WriteBatch memory usage: db=..., before=...MiB, after=...MiB` Verify that printed sizes (before and after) make sense. Reviewers: Fabien, O1 Bitcoin ABC, #bitcoin_abc, deadalnix Reviewed By: Fabien, O1 Bitcoin ABC, #bitcoin_abc Differential Revision: https://reviews.bitcoinabc.org/D3948

…te LevelDB memory use 741f017 Add DynamicMemoryUsage() to LevelDB (Evan Klitzke) Pull request description: This adds a new method `CDBWrapper::DynamicMemoryUsage()` similar to Bitcoin's existing methods of the same name. It's implemented by asking LevelDB for the information, and then parsing the string response. I've also added logging to `CDBWrapper::WriteBatch()` to track this information: ``` $ tail -f ~/.bitcoin/testnet3/debug.log | grep WriteBatch 2018-03-05 19:34:55 WriteBatch memory usage: db=chainstate, before=0.0MiB, after=0.0MiB 2018-03-05 19:35:17 WriteBatch memory usage: db=index, before=0.0MiB, after=0.0MiB 2018-03-05 19:35:17 WriteBatch memory usage: db=chainstate, before=0.0MiB, after=8.0MiB 2018-03-05 19:35:22 WriteBatch memory usage: db=index, before=0.0MiB, after=0.0MiB 2018-03-05 19:35:22 WriteBatch memory usage: db=chainstate, before=8.0MiB, after=17.0MiB 2018-03-05 19:35:26 WriteBatch memory usage: db=index, before=0.0MiB, after=0.0MiB 2018-03-05 19:35:27 WriteBatch memory usage: db=chainstate, before=9.0MiB, after=18.0MiB 2018-03-05 19:35:40 WriteBatch memory usage: db=index, before=0.0MiB, after=0.0MiB 2018-03-05 19:35:41 WriteBatch memory usage: db=chainstate, before=9.0MiB, after=7.0MiB 2018-03-05 19:35:52 WriteBatch memory usage: db=index, before=0.0MiB, after=0.0MiB 2018-03-05 19:35:52 WriteBatch memory usage: db=chainstate, before=7.0MiB, after=9.0MiB ^C ``` As LevelDB doesn't seem to provide a way to get the database name, I've also added a new `m_name` field to the `CDBWrapper`. This is necessary because we have multiple LevelDB databases (two now, and possibly more later, e.g. bitcoin#11857). I am using this information in other branches where I'm experimenting with changing LevelDB buffer sizes. Tree-SHA512: 7ea8ff5484bb07ef806af17d000c74ccca27d2e0f6c3229e12d93818f00874553335d87428482bd8acbcae81ea35aef2a243326f9fccbfac25989323d24391b4

fanquake added the Resource usage label Mar 5, 2018

laanwj reviewed Mar 5, 2018

View reviewed changes

eklitzke force-pushed the leveldb_memory branch from d7bb505 to e7fa704 Compare March 5, 2018 20:28

promag reviewed Mar 5, 2018

View reviewed changes

sipa reviewed Mar 5, 2018

View reviewed changes

eklitzke force-pushed the leveldb_memory branch from 2468f3d to 8f1026b Compare March 5, 2018 21:24

ryanofsky reviewed Mar 5, 2018

View reviewed changes

Add DynamicMemoryUsage() to LevelDB

741f017

This adds a DynamicMemoryUsage() method similar to the existing methods of the same name, and adds logging of memory usage to CDBWrapper::WriteBatch.

eklitzke force-pushed the leveldb_memory branch from 8f1026b to 741f017 Compare March 6, 2018 05:26

ryanofsky reviewed Mar 6, 2018

View reviewed changes

laanwj merged commit 741f017 into bitcoin:master Mar 6, 2018

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Add DynamicMemoryUsage() to CDBWrapper to estimate LevelDB memory use #12604

Add DynamicMemoryUsage() to CDBWrapper to estimate LevelDB memory use #12604

Uh oh!

Conversation

eklitzke commented Mar 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag commented Mar 5, 2018

Uh oh!

laanwj commented Mar 5, 2018

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eklitzke commented Mar 6, 2018

Uh oh!

jonasschnelli commented Mar 6, 2018

Uh oh!

practicalswift commented Mar 6, 2018

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants