New stable, fixed-length cache keys #9126

pdillinger · 2021-11-04T05:54:20Z

Summary: This change standardizes on a new 16-byte cache key format for
block cache (incl compressed and secondary) and persistent cache (but
not table cache and row cache).

The goal is a really fast cache key with practically ideal stability and
uniqueness properties without external dependencies (e.g. from FileSystem).
A fixed key size of 16 bytes should enable future optimizations to the
concurrent hash table for block cache, which is a heavy CPU user /
bottleneck, but there appears to be measurable performance improvement
even with no changes to LRUCache.

This change replaces a lot of disjointed and ugly code handling cache
keys with calls to a simple, clean new internal API (cache_key.h).
(Preserving the old cache key logic under an option would be very ugly
and likely negate the performance gain of the new approach. Complete
replacement carries some inherent risk, but I think that's acceptable
with sufficient analysis and testing.)

The scheme for encoding new cache keys is complicated but explained
in cache_key.cc.

Also: EndianSwapValue is moved to math.h to be next to other bit
operations. (Explains some new include "math.h".) ReverseBits operation
added and unit tests added to hash_test for both.

Fixes #7405 (presuming a root cause)

Test Plan:

Basic correctness

Several tests needed updates to work with the new functionality, mostly
because we are no longer relying on filesystem for stable cache keys
so table builders & readers need more context info to agree on cache
keys. This functionality is so core, a huge number of existing tests
exercise the cache key functionality.

Performance

Create db with
TEST_TMPDIR=/dev/shm ./db_bench -bloom_bits=10 -benchmarks=fillrandom -num=3000000 -partition_index_and_filters
And test performance with
TEST_TMPDIR=/dev/shm ./db_bench -readonly -use_existing_db -bloom_bits=10 -benchmarks=readrandom -num=3000000 -duration=30 -cache_index_and_filter_blocks -cache_size=250000 -threads=4
using DEBUG_LEVEL=0 and simultaneous before & after runs.
Before ops/sec, avg over 100 runs: 121924
After ops/sec, avg over 100 runs: 125385 (+2.8%)

Collision probability

I have built a tool, ./cache_bench -stress_cache_key to broadly simulate host-wide cache activity
over many months, by making some pessimistic simplifying assumptions:

Every generated file has a cache entry for every byte offset in the file (contiguous range of cache keys)
All of every file is cached for its entire lifetime

We use a simple table with skewed address assignment and replacement on address collision
to simulate files coming & going, with quite a variance (super-Poisson) in ages. Some output
with ./cache_bench -stress_cache_key -sck_keep_bits=40:

Total cache or DBs size: 32TiB  Writing 925.926 MiB/s or 76.2939TiB/day
Multiply by 9.22337e+18 to correct for simulation losses (but still assume whole file cached)

These come from default settings of 2.5M files per day of 32 MB each, and
-sck_keep_bits=40 means that to represent a single file, we are only keeping 40 bits of
the 128-bit cache key. With file size of 2**25 contiguous keys (pessimistic), our simulation
is about 2**(128-40-25) or about 9 billion billion times more prone to collision than reality.

More default assumptions, relatively pessimistic:

100 DBs in same process (doesn't matter much)
Re-open DB in same process (new session ID related to old session ID) on average
every 100 files generated
Restart process (all new session IDs unrelated to old) 24 times per day

After enough data, we get a result at the end:

(keep 40 bits)  17 collisions after 2 x 90 days, est 10.5882 days between (9.76592e+19 corrected)

If we believe the (pessimistic) simulation and the mathematical generalization, we would need to run a billion machines all for 97 billion days to expect a cache key collision. To help verify that our generalization ("corrected") is robust, we can make our simulation more precise with -sck_keep_bits=41 and 42, which takes more running time to get enough data:

(keep 41 bits)  16 collisions after 4 x 90 days, est 22.5 days between (1.03763e+20 corrected)
(keep 42 bits)  19 collisions after 10 x 90 days, est 47.3684 days between (1.09224e+20 corrected)

The generalized prediction still holds. With the -sck_randomize option, we can see that we are beating "random" cache keys (except offsets still non-randomized) by a modest amount (roughly 20x less collision prone than random), which should make us reasonably comfortable even in "degenerate" cases:

197 collisions after 1 x 90 days, est 0.456853 days between (4.21372e+18 corrected)

I've run other tests to validate other conditions behave as expected, never behaving "worse than random" unless we start chopping off structured data.

Summary: This change standardizes on a new 16-byte cache key format for block cache (incl compressed and secondary) and persistent cache (but not table cache and row cache). The goal is a really fast cache key with practically ideal stability and uniqueness properties. A fixed key size of 16 bytes should enable future optimizations to the concurrent hash table for block cache, which is a heavy CPU user / bottleneck. TODO: more Test Plan: TODO

zhichao-cao · 2021-11-08T23:27:02Z

The QPS improvement is explicit. Looks like the legacy cache key generation really causes kind of heavy CPU overhead.

zhichao-cao · 2021-11-08T23:55:16Z

cache/cache_key.h

+  // max_offset is based on file size (see WithOffset) and is required here to
+  // choose an appropriate (sub-)encoding. This constructor never generates an
+  // "empty" base key.
+  OffsetableCacheKey(const std::string &db_id, const std::string &db_session_id,


Basically, the OffsetableCacheKey is used per SST file. For different SST file, we need to have different OffsetableCacheKey instance?

Yes, one instance for each SST file. So even though there's some hashing and digit parsing for building OffsetableCacheKey, this only happens on table file open. The WithOffset step is really fast.

zhichao-cao · 2021-11-09T00:04:59Z

cache/cache_key.h

+//
+// This class "is a" CacheKey only privately so that it is not misused as
+// a ready-to-use CacheKey.
+class OffsetableCacheKey : private CacheKey {


If we close and reopen an DB, even block cache and secondary cache has no change, the cache key for the same block in the same SST file will be different right?

For the newer SST file case, it is the same since we use the session, file, and db_id from the file property. But if it is blocks from existing SST file, we will not be able to find them in both cache.

It will be stable on newer SST files. See BlockBasedTable::SetupBaseCacheKey.

zhichao-cao · 2021-11-10T19:49:53Z

table/block_based/block_based_table_reader.cc

+    // value because of the case of VersionSet::Recover opening some table
+    // files and later setting the DB ID. So we just rely on uniqueness
+    // level provided by session ID.
+    db_id = "unknown";


Should we make "unknown" as a static extern? Maybe it will be used in more places

zhichao-cao · 2021-11-10T19:59:26Z

table/block_based/block_based_table_reader.cc

-                         rep_->compressed_cache_key_prefix_size, handle,
-                         compressed_cache_key);
-    }
+    key_data = GetCacheKey(rep_->base_cache_key, handle);


Should we add a unit test to check the current scheme handles both compressed and regular key in a desired way?

zhichao-cao

In general, the PR looks good to me. Just left 2 minor comments.

mrambacher · 2021-11-08T20:05:39Z

cache/cache_key.h

+// CacheKeys are currently used by calling AsSlice() to pass as a key to
+// Cache. For performance, the keys are endianness-dependent (though otherwise
+// portable). (Persistable cache entries are not intended to cross platforms.)
+class CacheKey {


Would this be more efficient to move the two fields in a struct and have the Slice be created once (rather than on every call)?

Since we are generally calling a virtual function expecting Slice& (not Slice), your suggestion is probably more efficient. 👍

I was probably thinking ahead to a future when block cache can take CacheKey& as key. :)

Actually, on construction, we usually use the CacheKey once. If you want to use it more than once, you can save the Slice.

mrambacher · 2021-11-10T22:42:35Z

cache/cache_key.h

+  // max_offset is based on file size (see WithOffset) and is required here to
+  // choose an appropriate (sub-)encoding. This constructor never generates an
+  // "empty" base key.
+  OffsetableCacheKey(const std::string &db_id, const std::string &db_session_id,


For table keys, should they also contain the "compression type" (in addition to the offset)? What happens if the block_cache == block_cache_compressed -- wouldn't both the compressed and uncompressed keys map to the same key and chaos would ensue? I do not think there is anything preventing both types of cache from being the same.

Good observation. I guess I didn't think of that because it would be such a bad configuration under the current caching infrastructure. (Compressed cache entries would be completely useless and crowd out uncompressed entries, because uncompressed entries would get any hits first. Compressed entries guaranteed ejected before (or immediately adjacent to) corresponding uncompressed entry.)

Long term, I would like to see the Cache 'typed' so that the type of entry is sufficient to ensure logical uniqueness.

Actually, this is a pre-existing issue and this change does not make it better or worse. I've posted #9172 for protecting against the problem though.

pdillinger · 2021-11-11T18:27:56Z

My main concern with this design is the disconnect between cache keys and SST unique IDs (still marked "experimental"). Although both are based on session id, db id, and file number, this cache key "squeezes together" the session id and file number to ensure space in 128 bits for the offset within the file. In extreme cases, this "squeezing together" can harm the uniqueness to where it's not really using the full power of the 128 bits. The cache key encoding is also dependent on file size (in extreme cases), which is not available in TableProperties, which is a convenient precursor for getting SST unique id.

Possible solutions:

A more clever encoding encoding of the data that works well as both unique ID and as base cache key.
Instead of awkwardly combining session id and file number, use a SemiStructuredUniqueIdGen to assign 128-bit unique ids to generated files. Such a value would be ideal[*] both as file unique ID and as base cache key. But this would be YET ANOTHER piece of metadata associated with an SST file. (If we store this unique ID in the Footer, it could also be used for base context checksum in format_version=6 and context-aware block checksums #9058, when cut down to 32 bits.)

[*] Assuming no coordination between processes running RocksDB is relied upon. See https://github.com/pdillinger/unique_id

pdillinger · 2021-11-11T18:29:02Z

A more clever encoding encoding of the data that works well as both unique ID and as base cache key.

But I seem to be running up against the limits of clever, and clever carries complexity.

pdillinger · 2021-11-11T21:28:50Z

use a SemiStructuredUniqueIdGen to assign 128-bit unique ids to generated files. Such a value would be ideal

... except it would mean that in order to have a distinctive set of cache key prefixes for a DB, we would need a prefix for each SST file rather than (approximately) one prefix per original session ID (among live files). This is probably not a big deal because cache dumping should be a rare operation, and we could always represent the set with a Bloom(-like) filter if we want extra space & time savings.

pdillinger · 2021-11-18T21:11:43Z

I just modified / simplified to a fixed 24-byte cache key, and performance was worse than baseline: 76615.1 ops/sec vs. 79015.6

facebook-github-bot · 2021-12-16T21:50:43Z

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

mrambacher · 2021-12-16T21:54:10Z

cache/cache_bench_tool.cc

+DEFINE_bool(stress_cache_key, false, "If true, run cache key stress test instead");
+DEFINE_uint32(sck_files_per_day, 2500000,
+              "(-stress_cache_key) Simulated files generated per day");
+DEFINE_uint32(sck_duration, 90,


Can you call this something other than "duration"? Duration in db_bench means how long to run the tests whereas here it means how many days to simulate. "sck_days" or something like that might be a better name.

mrambacher · 2021-12-16T21:56:04Z

cache/cache_bench_tool.cc

+    sck_db_count, 100,
+    "(-stress_cache_key) Parallel DBs in operation");
+DEFINE_uint32(sck_table_bits, 20,
+              "(-stress_cache_key) Log2 number of tracked files");


Please provide a better description. I am not sure what "table_bits" has to do with "number of tracked files".

mrambacher · 2021-12-16T21:56:34Z

cache/cache_bench_tool.cc

+              "(-stress_cache_key) Log2 number of tracked files");
+DEFINE_uint32(
+    sck_keep_bits, 50,
+    "(-stress_cache_key) Number of cache key bits to keep");


Please provide a better description. I am not sure what "number of bits to keep" is supposed to mean.

mrambacher · 2021-12-16T22:08:26Z

cache/cache_bench_tool.cc

+        stripped = GetSliceHash64(ck.AsSlice()) >> shift_away;
+      } else {
+        uint32_t a = DecodeFixed32(ck.AsSlice().data()) << shift_away_a;
+        uint32_t b = DecodeFixed32(ck.AsSlice().data() + 12) >> shift_away_b;


Would it be better to have a Slice cks = ck.AsSlice()? Wondering the overhead of creating/using extra slices.

mrambacher · 2021-12-16T22:09:18Z

cache/cache_bench_tool.cc

+      } else {
+        uint32_t a = DecodeFixed32(ck.AsSlice().data()) << shift_away_a;
+        uint32_t b = DecodeFixed32(ck.AsSlice().data() + 12) >> shift_away_b;
+        stripped = (uint64_t{a} << 32) + b;


Is there a hashing method that combines two values we should be using instead?

mrambacher · 2021-12-16T22:10:06Z

cache/cache_bench_tool.cc

+      }
+      if (stripped == 0) {
+        // Unlikely, but we need to exclude tracking this value
+        printf("Hit Zero!                                                  \n");


Should this be to stderr instead? Wondering if it will get lost if it is hit...

mrambacher · 2021-12-16T22:12:38Z

cache/cache_bench_tool.cc

+        ResetProcess();
+      } else {
+        // Replace
+        table_[pos] = stripped;


Doesn't this just guarantee that two successive hashes to the same pos are not the same? Is it possible that there is an intervening one that would cause something like [A, B, A] where the collision was not detected?

This is simulation of various files and their cache entries kept for various lifetimes. There is no collision in practice if there's no overlap in lifetime (on disk UNION in cache).

mrambacher · 2021-12-16T22:27:18Z

cache/cache_bench_tool.cc

+    uint32_t shift_away = 64 - FLAGS_sck_keep_bits;
+    uint32_t shift_away_b = shift_away / 3;
+    uint32_t shift_away_a = shift_away - shift_away_b;


Please add a comment here to explain the math. Not sure what the shift_away / 3 is for.

mrambacher · 2021-12-16T22:29:09Z

cache/cache_bench_tool.cc

+    uint32_t report_count = 0;
+    uint32_t collisions_this_run = 0;
+    // Round robin through DBs
+    for (size_t db_i = 0;; ++db_i) {


Is there a reason to not also have this for conditional/update file_count?

pdillinger · 2021-12-16T23:26:56Z

@mrambacher My plan is to land this to get it going through downstream testing, with the simulation in a somewhat dirty state, and follow up with some improvements to comments, more unit tests, etc. Thanks for the input.

Summary: After facebook#9126 Test Plan: CI

Summary: After #9126 Pull Request resolved: #9309 Test Plan: CI Reviewed By: ajkr Differential Revision: D33188902 Pulled By: pdillinger fbshipit-source-id: 54bf34e33c2b30b1b8dc2a0229e84c194321b606

Summary: Follow-up to #9126 Added new unit tests to validate some of the claims of guaranteed uniqueness within certain large bounds. Also cleaned up the cache_bench -stress-cache-key tool with better comments and description. Pull Request resolved: #9329 Test Plan: no changes to production code Reviewed By: mrambacher Differential Revision: D33269328 Pulled By: pdillinger fbshipit-source-id: 3a2b684a6b2b15f79dc872e563e3d16563be26de

facebook-github-bot added the CLA Signed label Nov 4, 2021

pdillinger added 9 commits November 4, 2021 08:36

Fix ReverseBits test in hash_test

95113f8

Improve file prefix handling

ede7375

Remove debug output

02f4c3b

Some cleanup

24ebdf6

Merge remote-tracking branch 'origin/main' into new_cache_key

d5ef9fc

Readability improvements

f90a406

Update CacheEntryStatsCollector

a8d78f1

More comments, readability

c9feb5a

Fix^2 hash_test

a7405eb

pdillinger mentioned this pull request Nov 4, 2021

Make Cache into a Customizable Object #8998

Open

pdillinger added 6 commits November 4, 2021 15:25

An old Windows fix

1fa1a95

Merge remote-tracking branch 'origin/main' into new_cache_key

acf3c8c

Detailed comments

c049a27

Merge remote-tracking branch 'origin/main' into new_cache_key

d527296

TODO

9a5ed4a

More analysis in comments

1c45ab8

pdillinger changed the title ~~New stable, fixed-length cache keys~~ [RFC] New stable, fixed-length cache keys Nov 5, 2021

pdillinger marked this pull request as ready for review November 5, 2021 17:42

zhichao-cao reviewed Nov 8, 2021

View reviewed changes

zhichao-cao reviewed Nov 9, 2021

View reviewed changes

zhichao-cao reviewed Nov 10, 2021

View reviewed changes

zhichao-cao approved these changes Nov 10, 2021

View reviewed changes

mrambacher reviewed Nov 10, 2021

View reviewed changes

Merge remote-tracking branch 'origin/main' into new_cache_key

882ed7c

pdillinger mentioned this pull request Dec 14, 2021

format_version=6 and context-aware block checksums #9058

Closed

Merge remote-tracking branch 'origin/main' into new_cache_key

6851d96

pdillinger changed the title ~~[RFC] New stable, fixed-length cache keys~~ New stable, fixed-length cache keys Dec 16, 2021

pdillinger added 5 commits December 16, 2021 11:43

Simulation tool

7286fa2

Missed 'make format' on part of change

7020e4a

Hopefully fix clang-analyze issue; small simulator improvements

1b241d7

Merge remote-tracking branch 'origin/main' into new_cache_key

f8755b6

Update HISTORY.md

ba25f2d

mrambacher reviewed Dec 16, 2021

View reviewed changes

facebook-github-bot closed this in 0050a73 Dec 17, 2021

pdillinger added a commit to pdillinger/rocksdb that referenced this pull request Dec 17, 2021

Fix unity build with SUPPORT_CLOCK_CACHE

5e1ec02

Summary: After facebook#9126 Test Plan: CI

pdillinger mentioned this pull request Dec 17, 2021

Fix unity build with SUPPORT_CLOCK_CACHE #9309

Closed

pdillinger mentioned this pull request Dec 21, 2021

Enhance new cache key testing & comments #9329

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New stable, fixed-length cache keys #9126

New stable, fixed-length cache keys #9126

pdillinger commented Nov 4, 2021 •

edited

Loading

zhichao-cao commented Nov 8, 2021

zhichao-cao Nov 8, 2021

pdillinger Nov 9, 2021 •

edited

Loading

zhichao-cao Nov 9, 2021

zhichao-cao Nov 9, 2021

pdillinger Nov 9, 2021

zhichao-cao Nov 10, 2021

zhichao-cao Nov 10, 2021

zhichao-cao left a comment

mrambacher Nov 8, 2021

pdillinger Nov 11, 2021

pdillinger Nov 15, 2021

mrambacher Nov 10, 2021

pdillinger Nov 11, 2021

pdillinger Nov 16, 2021

pdillinger commented Nov 11, 2021

pdillinger commented Nov 11, 2021

pdillinger commented Nov 11, 2021

pdillinger commented Nov 18, 2021

facebook-github-bot commented Dec 16, 2021

mrambacher Dec 16, 2021

mrambacher Dec 16, 2021

mrambacher Dec 16, 2021

mrambacher Dec 16, 2021

mrambacher Dec 16, 2021

mrambacher Dec 16, 2021

mrambacher Dec 16, 2021

pdillinger Dec 16, 2021

mrambacher Dec 16, 2021

mrambacher Dec 16, 2021

pdillinger commented Dec 16, 2021

New stable, fixed-length cache keys #9126

New stable, fixed-length cache keys #9126

Conversation

pdillinger commented Nov 4, 2021 • edited Loading

Basic correctness

Performance

Collision probability

zhichao-cao commented Nov 8, 2021

Choose a reason for hiding this comment

pdillinger Nov 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhichao-cao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdillinger commented Nov 11, 2021

pdillinger commented Nov 11, 2021

pdillinger commented Nov 11, 2021

pdillinger commented Nov 18, 2021

facebook-github-bot commented Dec 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdillinger commented Dec 16, 2021

pdillinger commented Nov 4, 2021 •

edited

Loading

pdillinger Nov 9, 2021 •

edited

Loading