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

pull eklitzke wants to merge 1 commits into bitcoin:master from eklitzke:leveldb_memory changing 2 files +26 −0
  1. eklitzke commented at 7:41 pm on March 5, 2018: contributor

    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:

     0$ tail -f ~/.bitcoin/testnet3/debug.log | grep WriteBatch
 12018-03-05 19:34:55 WriteBatch memory usage: db=chainstate, before=0.0MiB, after=0.0MiB
 22018-03-05 19:35:17 WriteBatch memory usage: db=index, before=0.0MiB, after=0.0MiB
 32018-03-05 19:35:17 WriteBatch memory usage: db=chainstate, before=0.0MiB, after=8.0MiB
 42018-03-05 19:35:22 WriteBatch memory usage: db=index, before=0.0MiB, after=0.0MiB
 52018-03-05 19:35:22 WriteBatch memory usage: db=chainstate, before=8.0MiB, after=17.0MiB
 62018-03-05 19:35:26 WriteBatch memory usage: db=index, before=0.0MiB, after=0.0MiB
 72018-03-05 19:35:27 WriteBatch memory usage: db=chainstate, before=9.0MiB, after=18.0MiB
 82018-03-05 19:35:40 WriteBatch memory usage: db=index, before=0.0MiB, after=0.0MiB
 92018-03-05 19:35:41 WriteBatch memory usage: db=chainstate, before=9.0MiB, after=7.0MiB
102018-03-05 19:35:52 WriteBatch memory usage: db=index, before=0.0MiB, after=0.0MiB
112018-03-05 19:35:52 WriteBatch memory usage: db=chainstate, before=7.0MiB, after=9.0MiB
12^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.

  2. fanquake added the label Resource usage on Mar 5, 2018
  3. in src/dbwrapper.cpp:159 in d7bb505099 outdated 
    155@@ -155,11 +156,25 @@ CDBWrapper::~CDBWrapper()
156 
157 bool CDBWrapper::WriteBatch(CDBBatch& batch, bool fSync)
158 {
159+    double mem_before = DynamicMemoryUsage() / 1024 / 1024;
    laanwj commented at 8:23 pm on March 5, 2018:
    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.
    eklitzke commented at 8:28 pm on March 5, 2018:
    Good observation, fixed.
  4. eklitzke force-pushed on Mar 5, 2018
  5. in src/dbwrapper.cpp:171 in d7bb505099 outdated 
    166 }
167 
168+size_t CDBWrapper::DynamicMemoryUsage() {
169+    std::string memory;
170+    if (pdb->GetProperty("leveldb.approximate-memory-usage", &memory)) {
171+        return strtoul(memory.c_str(), nullptr, 10);
    promag commented at 8:30 pm on March 5, 2018:
    Use http://en.cppreference.com/w/cpp/string/basic_string/stoul instead.
    eklitzke commented at 8:58 pm on March 5, 2018:
    done
  6. in src/dbwrapper.h:291 in e7fa704cba outdated 
    286@@ -284,6 +287,9 @@ class CDBWrapper
287 
288     bool WriteBatch(CDBBatch& batch, bool fSync = false);
289 
290+    // Get an estimate of LevelDB memory usage (in bytes).
291+    size_t DynamicMemoryUsage();
    promag commented at 8:32 pm on March 5, 2018:
    Could be const?
    promag commented at 8:55 pm on March 5, 2018:
    Maybe it’s not possible, since GetProperty is not const
    eklitzke commented at 8:59 pm on March 5, 2018:
    Right, unless you want to give into the temptation of const cast.
    promag commented at 9:11 pm on March 5, 2018:
    Nah, IMO it’s fine as it is.
    ryanofsky commented at 10:22 pm on March 5, 2018:

    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.

  7. in src/dbwrapper.cpp:92 in e7fa704cba outdated 
    88@@ -89,6 +89,7 @@ static leveldb::Options GetOptions(size_t nCacheSize)
89 }
90 
91 CDBWrapper::CDBWrapper(const fs::path& path, size_t nCacheSize, bool fMemory, bool fWipe, bool obfuscate)
92+    :m_name(fs::basename(path))
    promag commented at 8:32 pm on March 5, 2018:
    Nit, space after :.
    promag commented at 8:43 pm on March 5, 2018:

    BTW, could use path in the debug instead?

    • relevant considering #11687
    • CDBWrapper constructor also logs path.
    eklitzke commented at 9:06 pm on March 5, 2018:

    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 #11687 since wallet files are not global like the other databases.
  8. in src/dbwrapper.cpp:160 in e7fa704cba outdated 
    155@@ -155,11 +156,28 @@ CDBWrapper::~CDBWrapper()
156 
157 bool CDBWrapper::WriteBatch(CDBBatch& batch, bool fSync)
158 {
159+    const bool log_memory = LogAcceptCategory(BCLog::LEVELDB);
160+    double mem_before;
    promag commented at 8:33 pm on March 5, 2018:
    Nit, initialize = 0.
    eklitzke commented at 9:03 pm on March 5, 2018:
    done (although I think it’s fine to keep unused variables uninitialized)
  9. in src/dbwrapper.cpp:161 in e7fa704cba outdated 
    155@@ -155,11 +156,28 @@ CDBWrapper::~CDBWrapper()
156 
157 bool CDBWrapper::WriteBatch(CDBBatch& batch, bool fSync)
158 {
159+    const bool log_memory = LogAcceptCategory(BCLog::LEVELDB);
160+    double mem_before;
161+    if (log_memory)
    promag commented at 8:34 pm on March 5, 2018:
    Nit, same line.
    sipa commented at 8:55 pm on March 5, 2018:
    Nit: braces whenever the then branch is not on the same line.
    eklitzke commented at 9:01 pm on March 5, 2018:
    done
    eklitzke commented at 9:02 pm on March 5, 2018:
    I added braces, which I think is what you mean here?
  10. in src/dbwrapper.cpp:165 in e7fa704cba outdated 
    160+    double mem_before;
161+    if (log_memory)
162+        mem_before = DynamicMemoryUsage() / 1024 / 1024;
163     leveldb::Status status = pdb->Write(fSync ? syncoptions : writeoptions, &batch.batch);
164     dbwrapper_private::HandleError(status);
165+    if (log_memory)
    promag commented at 8:34 pm on March 5, 2018:

    Nit

    0if (log_memory) {
1    double mem_after = ...;
2    LogPrint(BCLog::LEVELDB, ...);
3}
    eklitzke commented at 9:02 pm on March 5, 2018:
    done
  11. in src/dbwrapper.cpp:173 in e7fa704cba outdated 
    168     return true;
169 }
170 
171+size_t CDBWrapper::DynamicMemoryUsage() {
172+    std::string memory;
173+    if (pdb->GetProperty("leveldb.approximate-memory-usage", &memory)) {
    promag commented at 8:39 pm on March 5, 2018:

    Suggestion:

    0if (!pdb->GetProperty(...)) {
1    LogPrint(BCLog::LEVELDB, "Failed to get approximate-memory-usage property\n");
2    return 0;
3}
4
5return stoul(memory);
    eklitzke commented at 9:01 pm on March 5, 2018:
    done
  12. promag commented at 9:13 pm on March 5, 2018: member
    utACK after squash.
  13. laanwj commented at 9:17 pm on March 5, 2018: member
    utACK
  14. eklitzke force-pushed on Mar 5, 2018
  15. ryanofsky commented at 10:31 pm on March 5, 2018: member
    utACK e7fa704cbaa5de2ca9bfffe560ff6c6fdf751ac0
  16. Add DynamicMemoryUsage() to LevelDB 
    This adds a DynamicMemoryUsage() method similar to the existing methods
of the same name, and adds logging of memory usage to
CDBWrapper::WriteBatch.
    741f0177c5
  17. eklitzke force-pushed on Mar 6, 2018
  18. eklitzke commented at 5:27 am on March 6, 2018: contributor
    Updated with 741f0177c53ae536801a67c8ec194d6be3505d2d to fix const correctness.
  19. jonasschnelli commented at 5:44 am on March 6, 2018: contributor
    Nice! utACK 741f0177c53ae536801a67c8ec194d6be3505d2d
  20. practicalswift commented at 6:13 am on March 6, 2018: contributor
    utACK 741f0177c53ae536801a67c8ec194d6be3505d2d
  21. ryanofsky commented at 2:03 pm on March 6, 2018: member
    utACK 741f0177c53ae536801a67c8ec194d6be3505d2d. Differences since last review were various style cleanups that don’t change behavior.
  22. laanwj merged this on Mar 6, 2018
  23. laanwj closed this on Mar 6, 2018
  24. laanwj referenced this in commit 9903537750 on Mar 6, 2018
  25. Fabcien referenced this in commit 8b217943d2 on Aug 30, 2019
  26. jonspock referenced this in commit 967ae8b763 on Dec 8, 2019
  27. jonspock referenced this in commit 63aeab11db on Dec 8, 2019
  28. jonspock referenced this in commit fc8fc19747 on Dec 8, 2019
  29. proteanx referenced this in commit fa4fd9798e on Dec 12, 2019
  30. PastaPastaPasta referenced this in commit d1193cf4c4 on Jun 10, 2020
  31. PastaPastaPasta referenced this in commit 49089b7748 on Jun 12, 2020
  32. PastaPastaPasta referenced this in commit 3a7480f7c5 on Jun 13, 2020
  33. PastaPastaPasta referenced this in commit f25b9a7447 on Jun 14, 2020
  34. PastaPastaPasta referenced this in commit 9b825c8bb7 on Jun 14, 2020
  35. gades referenced this in commit 23fa5cdc7b on Jun 24, 2021
  36. DrahtBot locked this on Sep 8, 2021


eklitzke laanwj promag ryanofsky sipa jonasschnelli practicalswift

Labels
Resource usage

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-31 03:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me