Add a blob-specific cache priority #10309

gangliao · 2022-07-05T08:53:27Z

Summary:

RocksDB's Cache abstraction currently supports two priority levels for items: high (used for frequently accessed/highly valuable SST metablocks like index/filter blocks) and low (used for SST data blocks). Blobs are typically lower-value targets for caching than data blocks, since 1) with BlobDB, data blocks containing blob references conceptually form an index structure which has to be consulted before we can read the blob value, and 2) cached blobs represent only a single key-value, while cached data blocks generally contain multiple KVs. Since we would like to make it possible to use the same backing cache for the block cache and the blob cache, it would make sense to add a new, lower-than-low cache priority level (bottom level) for blobs so data blocks are prioritized over them.

This task is a part of #10156

gangliao · 2022-07-11T17:52:15Z

The basic idea of this PR is:

Add a new priority level, the BOTTOM level, for blob caching. HIGH > LOW > BOTTOM
add a new midpoint for bottom level

x x x x B x x x x x x x x x L x x x x x H
        |                   |
    mid-point           mid-point

cache/lru_cache.h

ltamasi

Thanks @gangliao for the patch! Please see some questions/comments below

cache/cache_bench_tool.cc

HISTORY.md

include/rocksdb/cache.h

cache/lru_cache.cc

cache/compressed_secondary_cache.cc

cache/lru_cache.cc

cache/lru_cache.h

cache/lru_cache_test.cc

include/rocksdb/cache.h

java/src/main/java/org/rocksdb/LRUCache.java

HISTORY.md

cache/compressed_secondary_cache_test.cc

cache/lru_cache.cc

facebook-github-bot · 2022-07-27T19:48:39Z

@gangliao has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-07-27T19:49:11Z

@gangliao has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

riversand963 · 2022-07-27T20:02:47Z

table/block_based/block_based_table_factory.cc

@@ -454,6 +454,7 @@ void BlockBasedTableFactory::InitializeOptions() {
    // It makes little sense to pay overhead for mid-point insertion while the
    // block size is only 8MB.
    co.high_pri_pool_ratio = 0.0;
+    co.low_pri_pool_ratio = 1.0;


Question: I see that in LRUCache, the default of low_pri_pool_ratio is 0.5, while it is 1.0 here. Am I missing anything?

Maybe use a constant to represent the default value?

@riversand963 's comment just made me realize that we would probably want to adjust the default of low_pri_pool_ratio for the purposes of backward compatibility. That is, I think what we would want is a default of say, -1, where negative values would be interpreted as "use (1 - high_pri_pool_ratio)". With the current default of 0.5, we run the risk of applications hitting errors during cache creation if they set high_pri_pool_ratio to a value greater than 0.5.

Alternatively, we could make negative values mean "unlimited low-priority pool capacity."

Side note: it would make sense for the pool capacity data members to be size_ts as opposed to doubles.

pdillinger · 2022-07-27T20:45:53Z

What does the performance test say? That needs to be in "test plan" for non-trivial changes to performance-critical structures

gangliao · 2022-07-27T20:48:01Z

What does the performance test say? That needs to be in "test plan" for non-trivial changes to performance-critical structures

Yeah, we will do a bunch of perf tests during this week.

facebook-github-bot · 2022-07-27T21:41:59Z

@gangliao has updated the pull request. You must reimport the pull request before landing.

ltamasi · 2022-07-27T22:05:16Z

include/rocksdb/cache.h

+        low_pri_pool_ratio(_low_pri_pool_ratio == kDefaultLowPriortyPoolRatio
+                               ? 1.0 - _high_pri_pool_ratio
+                               : _low_pri_pool_ratio),


I feel it would be nicer to deal with this inside LRUCache. Also, we could treat all out-of-range (< 0 or > 1) values as "use 1 - high pro pool ratio" (or alternatively, as "no limit on the size of the low-priority pool" as we discussed)

ltamasi · 2022-07-27T22:09:26Z

include/rocksdb/cache.h

+const double kDefaultLowPriortyPoolRatio = -1.0;
+


Defining a namespace-scope variable in the header like this can lead to ODR violations. Also, the variable name is misspelled (then again, the simplest solution would be to ditch this constant)

ltamasi · 2022-07-27T22:10:56Z

include/rocksdb/cache.h

+    assert(high_pri_pool_ratio >= 0.0 && high_pri_pool_ratio <= 1.0);
+    assert(low_pri_pool_ratio >= 0.0 && low_pri_pool_ratio <= 1.0);
+    assert(high_pri_pool_ratio + low_pri_pool_ratio <= 1.0);


I don't think these assertions are legit in the sense that they are not guaranteed hold when the object is created (the range checks are done in NewLRUCache IIRC)

gangliao · 2022-07-27T22:22:31Z

I'm not sure why there is a test failure. Even after rolling back to Sun Jul 17, the problem still exists. @ltamasi

 ./lru_cache_test --gtest_filter=DBSecondaryCacheTest.TestSecondaryCacheCorrectness1
Note: Google Test filter = DBSecondaryCacheTest.TestSecondaryCacheCorrectness1
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from DBSecondaryCacheTest
[ RUN      ] DBSecondaryCacheTest.TestSecondaryCacheCorrectness1
Assertion failed: (cache_), function BlobFileCache, file db/blob/blob_file_cache.cc, line 34.
fish: Job 1, './lru_cache_test --gtest_filter…' terminated by signal SIGABRT (Abort)

Okay, just locate the bug is from this PR.

facebook-github-bot · 2022-07-27T22:49:14Z

@gangliao has updated the pull request. You must reimport the pull request before landing.

cache/lru_cache.cc

facebook-github-bot · 2022-07-28T00:03:05Z

@gangliao has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-07-28T00:16:09Z

@gangliao has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-07-28T00:21:13Z

@gangliao has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

ltamasi · 2022-07-28T17:02:22Z

What does the performance test say? That needs to be in "test plan" for non-trivial changes to performance-critical structures

Yeah, we will do a bunch of perf tests during this week.

I believe what @pdillinger was referring to above is making sure the change does not cause a performance regression. The way to do that would be to compare db_bench results with and without the change.

Summary: This reverts commit 8d17809 because of a clear performance regression seen in internal dashboard https://fburl.com/unidash/tpz75iee

ltamasi · 2022-07-29T13:52:53Z

cache/lru_cache.cc

-  } else {
-    // Insert "e" to the head of low-pri pool. Note that when
-    // high_pri_pool_ratio is 0, head of low-pri pool is also head of LRU list.
+  } else if (low_pri_pool_ratio_ > 0 && (e->IsLowPri() || e->HasHit())) {


So I'm fairly sure this is the line which causes the regression (see #10434 ). I think we have a bug causing priority inversion here: if there is no high-priority pool configured but there is a low-priority pool, high-priority items can end up in the bottom-priority pool. If so, it should be an easy fix

Summary: This reverts commit 8d17809 because of a clear performance regression seen in internal dashboard https://fburl.com/unidash/tpz75iee Pull Request resolved: #10434 Reviewed By: ltamasi Differential Revision: D38256373 Pulled By: pdillinger fbshipit-source-id: 134aa00f50dd7b1bbe037c227884a351342ec44b

facebook-github-bot added the CLA Signed label Jul 5, 2022

gangliao changed the title ~~Add a blob-specific cache priority~~ [Draft] Add a blob-specific cache priority Jul 5, 2022

gangliao mentioned this pull request Jul 8, 2022

BlobDB Caching #10156

Closed

14 tasks

gangliao changed the title ~~[Draft] Add a blob-specific cache priority~~ Add a blob-specific cache priority Jul 9, 2022

gangliao requested review from ltamasi, riversand963 and akankshamahajan15 July 11, 2022 17:52

ltamasi reviewed Jul 13, 2022

View reviewed changes

cache/lru_cache.h Show resolved Hide resolved

gangliao force-pushed the cache_pri branch 2 times, most recently from bdadb6e to 3d94420 Compare July 19, 2022 21:44