Only allocate a LevelDB block cache if LevelDB will actually use it #12825

1. 
   eklitzke commented at 6:18 am on March 29, 2018: contributor

   When snappy compression is disabled, LevelDB does not use a block cache. This PR avoids wasting memory reserved for the block cache in that situation, and gives it to the write buffer instead.

   Unfortunately this behavior is not documented (I have filed a bug upstream already). You can see here that PosixMmapReadableFile takes a scratch argument that is unused. This isn’t by accident; the return value of the method is compared against the supplied buffer here to determine whether the block cache should be used.

   Giving more memory to the write buffer is good for a number of reasons. Among them: LevelDB checks reads against the mem table holding current writes, so the write buffer acts as an LRU within LevelDB.

2. fanquake added the label Block storage on Mar 29, 2018
3. fanquake added the label Resource usage on Mar 29, 2018
4. 
   fanquake commented at 1:41 pm on March 29, 2018: member

   #12495 has now been merged.
5. jnewbery added the label UTXO Db and Indexes on Mar 29, 2018
6. jnewbery removed the label Block storage on Mar 29, 2018
7. 
   laanwj commented at 5:02 pm on April 7, 2018: member

   utACK https://github.com/bitcoin/bitcoin/pull/12825/commits/607e67fffb496d4b3bf2f2e01cea964474e54e16 needs rebase to get rid of the already-merged commit
8. MarcoFalke force-pushed on Apr 7, 2018
9. 
   MarcoFalke commented at 7:36 pm on April 7, 2018: member

   Poked GitHub to get rid of the commit without rebase:

   0git push git@github.com:eklitzke/bitcoin.git f9026afe12518bae3a8d700b2c7aff5522644405:buffer-cache
   1git push git@github.com:eklitzke/bitcoin.git 607e67f:buffer-cache -f
   

10. Only give LevelDB a block cache if LevelDB will actually use it.

    On systems that use mmap LevelDB does not use a block cache. On these systems we
    give the memory instead to the write buffer, where it will be used more
    effectively.

    09adcfdebb
11. eklitzke force-pushed on Apr 18, 2018
12. 
    eklitzke commented at 11:37 pm on April 18, 2018: contributor

    I have rebased this branch with master.
13. in src/dbwrapper.cpp:76 in 09adcfdebb outdated

    71@@ -72,6 +72,20 @@ class CBitcoinLevelDBLogger : public leveldb::Logger {
    72     }
    73 };
    74 
    75+// Mmap is only used on 64-bit Unix systems.
    76+constexpr bool LevelDBUsesMmap() {
    

    ---

    ---

    

    promag commented at 3:49 pm on April 19, 2018:

    Nit, { in newline.

    ---

    

    eklitzke commented at 7:16 pm on April 19, 2018:

    fixed
14. in src/dbwrapper.cpp:85 in 09adcfdebb outdated

    80+    return sizeof(void*) >= 8;
    81+#endif
    82+}
    83+
    84+// Systems with mmap do not use the block cache.
    85+constexpr bool LevelDBUsesBlockCache() {
    

    ---

    ---

    

    promag commented at 3:50 pm on April 19, 2018:

    Nit, { in newline.

    ---

    

    eklitzke commented at 7:16 pm on April 19, 2018:

    fixed
15. in src/dbwrapper.cpp:125 in 09adcfdebb outdated

    122+    // page cache), so don't bother in that case.
    123+    //
    124+    // Up to two write buffers may be held in memory simultaneously, so set
    125+    // write_buffer_size for the worst case.
    126+    if (LevelDBUsesBlockCache()) {
    127+        options.write_buffer_size = nCacheSize / 4;
    

    ---

    ---

    

    promag commented at 3:50 pm on April 19, 2018:

    Nit, keep order?
16. in src/dbwrapper.cpp:77 in 09adcfdebb outdated

    71@@ -72,6 +72,20 @@ class CBitcoinLevelDBLogger : public leveldb::Logger {
    72     }
    73 };
    74 
    75+// Mmap is only used on 64-bit Unix systems.
    76+constexpr bool LevelDBUsesMmap() {
    77+#ifdef WIN32
    

    ---

    ---

    

    promag commented at 3:56 pm on April 19, 2018:

    Could use __LP64__ macro instead? See https://stackoverflow.com/a/682955.

    ---

    

    eklitzke commented at 7:18 pm on April 19, 2018:

    It sounds like that refers to the data model based on the name, e.g. it sounds like you would have __LLP64__ on a 64-bit system using an LLP integer model. Additionally the LevelDB code uses sizeof in its check, so the code as-is is definitely consistent with the logic used within LevelDB.

    ---

    

    promag commented at 7:23 pm on April 19, 2018:

    That’s reasonable.
17. 
    promag commented at 3:58 pm on April 19, 2018: member

    utACK 09adcfd, after a little investigation change LGTM.

    For reference https://github.com/google/leveldb/issues/561.

18. fix braces 133534cf4c
19. 
    DrahtBot commented at 12:54 pm on July 22, 2018: member
20. DrahtBot closed this on Jul 22, 2018

    ---

21. DrahtBot reopened this on Jul 22, 2018

    ---

22. 
    fanquake commented at 2:28 pm on July 22, 2018: member

    Rebased / squashed in #13741.
23. fanquake closed this on Jul 22, 2018

    ---

24. MarcoFalke locked this on Sep 8, 2021

Contributors
eklitzke fanquake laanwj MarcoFalke promag DrahtBot

Labels
UTXO Db and Indexes Resource usage