From b52620ab0ea049cb0e6c17b09779065337662e01 Mon Sep 17 00:00:00 2001 From: Guido Tagliavini Ponce Date: Thu, 23 Jun 2022 11:26:50 -0700 Subject: [PATCH] Fix key size in cache_bench (#10234) Summary: cache_bench wasn't generating 16B keys, which are necessary for FastLRUCache. Also: - Added asserts in cache_bench, which is assuming that inserts never fail. When they fail (for example, if we used keys of the wrong size), memory allocated to the values will becomes leaked, and eventually the program crashes. - Move kCacheKeySize to the right spot. Pull Request resolved: https://github.com/facebook/rocksdb/pull/10234 Test Plan: ``make -j24 check``. Also, run cache_bench with FastLRUCache and check that memory usage doesn't blow up: ``./cache_bench -cache_type=fast_lru_cache -num_shard_bits=6 -skewed=true \ -lookup_insert_percent=100 -lookup_percent=0 -insert_percent=0 -erase_percent=0 \ -populate_cache=true -cache_size=1073741824 -ops_per_thread=10000000 \ -value_bytes=8192 -resident_ratio=1 -threads=16`` Reviewed By: pdillinger Differential Revision: D37382949 Pulled By: guidotag fbshipit-source-id: b697a942ebb215de5d341f98dc8566763436ba9b --- cache/cache_bench_tool.cc | 19 ++++++++++++------- cache/cache_key.h | 2 ++ cache/fast_lru_cache.h | 8 +++----- 3 files changed, 17 insertions(+), 12 deletions(-) diff --git a/cache/cache_bench_tool.cc b/cache/cache_bench_tool.cc index 2d5c35340..87245c0b3 100644 --- a/cache/cache_bench_tool.cc +++ b/cache/cache_bench_tool.cc @@ -3,6 +3,7 @@ // COPYING file in the root directory) and Apache 2.0 License // (found in the LICENSE.Apache file in the root directory). +#include "cache_key.h" #ifdef GFLAGS #include #include @@ -214,7 +215,8 @@ struct KeyGen { EncodeFixed64(key_data + 10, key); key_data[18] = char{4}; EncodeFixed64(key_data + 19, key); - return Slice(&key_data[off], sizeof(key_data) - off); + assert(27 >= kCacheKeySize); + return Slice(&key_data[off], kCacheKeySize); } }; @@ -321,8 +323,9 @@ class CacheBench { Random64 rnd(1); KeyGen keygen; for (uint64_t i = 0; i < 2 * FLAGS_cache_size; i += FLAGS_value_bytes) { - cache_->Insert(keygen.GetRand(rnd, max_key_, max_log_), createValue(rnd), - &helper1, FLAGS_value_bytes); + Status s = cache_->Insert(keygen.GetRand(rnd, max_key_, max_log_), + createValue(rnd), &helper1, FLAGS_value_bytes); + assert(s.ok()); } } @@ -542,8 +545,9 @@ class CacheBench { FLAGS_value_bytes); } else { // do insert - cache_->Insert(key, createValue(thread->rnd), &helper2, - FLAGS_value_bytes, &handle); + Status s = cache_->Insert(key, createValue(thread->rnd), &helper2, + FLAGS_value_bytes, &handle); + assert(s.ok()); } } else if (random_op < insert_threshold_) { if (handle) { @@ -551,8 +555,9 @@ class CacheBench { handle = nullptr; } // do insert - cache_->Insert(key, createValue(thread->rnd), &helper3, - FLAGS_value_bytes, &handle); + Status s = cache_->Insert(key, createValue(thread->rnd), &helper3, + FLAGS_value_bytes, &handle); + assert(s.ok()); } else if (random_op < lookup_threshold_) { if (handle) { cache_->Release(handle); diff --git a/cache/cache_key.h b/cache/cache_key.h index 525b13d90..0858f3c8e 100644 --- a/cache/cache_key.h +++ b/cache/cache_key.h @@ -65,6 +65,8 @@ class CacheKey { uint64_t offset_etc64_; }; +constexpr uint8_t kCacheKeySize = static_cast(sizeof(CacheKey)); + // A file-specific generator of cache keys, sometimes referred to as the // "base" cache key for a file because all the cache keys for various offsets // within the file are computed using simple arithmetic. The basis for the diff --git a/cache/fast_lru_cache.h b/cache/fast_lru_cache.h index e628895a4..9fcd694f2 100644 --- a/cache/fast_lru_cache.h +++ b/cache/fast_lru_cache.h @@ -22,10 +22,11 @@ #include "util/distributed_mutex.h" namespace ROCKSDB_NAMESPACE { + namespace fast_lru_cache { // LRU cache implementation using an open-address hash table. - +// // Every slot in the hash table is an LRUHandle. Because handles can be // referenced externally, we can't discard them immediately once they are // deleted (via a delete or an LRU eviction) or replaced by a new version @@ -51,7 +52,7 @@ namespace fast_lru_cache { // - Not R --> R: When an unreferenced element becomes referenced. This can only // happen if the element is V, since references to an element can only be // created when it's visible. - +// // Internally, the cache uses an open-addressed hash table to index the handles. // We use tombstone counters to keep track of displacements. // Because of the tombstones and the two possible visibility states of an @@ -70,9 +71,6 @@ namespace fast_lru_cache { // slot. In any case, the slot becomes available. When a handle is inserted // into that slot, it becomes a visible element again. -constexpr uint8_t kCacheKeySize = - static_cast(sizeof(ROCKSDB_NAMESPACE::CacheKey)); - // The load factor p is a real number in (0, 1) such that at all // times at most a fraction p of all slots, without counting tombstones, // are occupied by elements. This means that the probability that a