From 4067acabca0625dcb581a1e4497998c91c58ae1d Mon Sep 17 00:00:00 2001 From: Peter Dillinger Date: Thu, 18 May 2023 20:40:19 -0700 Subject: [PATCH] Compatibility step for separating BlockCache and GeneralCache APIs (#11450) Summary: Add two type aliases for Cache: BlockCache and GeneralCache, and add LRUCacheOptions::MakeSharedGeneralCache(). This will ease upgrade to an intended future change to separate the cache API between block cache and other (general) uses, including row cache. Separating the APIs will make it easier to expose more details of block caching for customization. For example, it would be nice to pass the file unique ID and offset as the logical cache key instead of using a Slice, which could facilitate some file-specific customizations in block cache. This would also make it clear that HyperClockCache is not usable as a general cache, because it can only deal with fixed-size block cache keys. block_cache, row_cache, and blob_cache are the uses of Cache in the public API. blob_cache should be able to use BlockCache while row_cache is a GeneralCache user, as its keys are of arbitrary size. Pull Request resolved: https://github.com/facebook/rocksdb/pull/11450 Test Plan: see updated unit test. Reviewed By: anand1976 Differential Revision: D45882067 Pulled By: pdillinger fbshipit-source-id: ff5d9f0b644f87ae337a29a7728ce3ed07b2a4b2 --- cache/lru_cache.cc | 9 +++++++++ db/db_test.cc | 15 ++++++++++++++- include/rocksdb/cache.h | 27 ++++++++++++++++++++++++++- include/rocksdb/options.h | 2 +- 4 files changed, 50 insertions(+), 3 deletions(-) diff --git a/cache/lru_cache.cc b/cache/lru_cache.cc index 036956db3..6284c87b5 100644 --- a/cache/lru_cache.cc +++ b/cache/lru_cache.cc @@ -711,4 +711,13 @@ std::shared_ptr LRUCacheOptions::MakeSharedCache() const { } return cache; } + +std::shared_ptr LRUCacheOptions::MakeSharedGeneralCache() const { + if (secondary_cache) { + // Not allowed for a GeneralCache + return nullptr; + } + // Works while GeneralCache is an alias for Cache + return MakeSharedCache(); +} } // namespace ROCKSDB_NAMESPACE diff --git a/db/db_test.cc b/db/db_test.cc index d23daa55d..cb0a41906 100644 --- a/db/db_test.cc +++ b/db/db_test.cc @@ -6894,7 +6894,20 @@ TEST_F(DBTest, CreateColumnFamilyShouldFailOnIncompatibleOptions) { TEST_F(DBTest, RowCache) { Options options = CurrentOptions(); options.statistics = ROCKSDB_NAMESPACE::CreateDBStatistics(); - options.row_cache = NewLRUCache(8192); + LRUCacheOptions cache_options; + cache_options.capacity = 8192; + options.row_cache = cache_options.MakeSharedGeneralCache(); + // BEGIN check that Cache classes as aliases of each other. + // Currently, GeneralCache and BlockCache are aliases for Cache. + // This is expected to change (carefully, intentionally) + std::shared_ptr general_cache = options.row_cache; + std::shared_ptr cache = general_cache; + std::shared_ptr block_cache = general_cache; + general_cache = cache; + block_cache = cache; + general_cache = block_cache; + cache = block_cache; + // END check that Cache classes as aliases of each other. DestroyAndReopen(options); ASSERT_OK(Put("foo", "bar")); diff --git a/include/rocksdb/cache.h b/include/rocksdb/cache.h index 3c49bf322..b64ad7014 100644 --- a/include/rocksdb/cache.h +++ b/include/rocksdb/cache.h @@ -25,6 +25,26 @@ class Cache; // defined in advanced_cache.h struct ConfigOptions; class SecondaryCache; +// These definitions begin source compatibility for a future change in which +// a specific class for block cache is split away from general caches, so that +// the block cache API can continue to become more specialized and +// customizeable, including in ways incompatible with a general cache. For +// example, HyperClockCache is not usable as a general cache because it expects +// only fixed-size block cache keys, but this limitation is not yet reflected +// in the API function signatures. +// * Phase 1 (done) - Make both BlockCache and GeneralCache aliases for Cache, +// and make a factory function for general caches. Encourage users of row_cache +// (not common) to switch to the factory function for general caches. +// * Phase 2 - Split off GenericCache as its own class, removing secondary +// cache support features and more from the API to simplify it. Between Phase 1 +// and Phase 2 users of row_cache will need to update their code. Any time +// after Phase 2, the block cache API can become more specialized in ways +// incompatible with general caches. +// * Phase 3 - Move existing RocksDB uses of Cache to BlockCache, and deprecate +// (but not yet remove) Cache as an alias for BlockCache. +using BlockCache = Cache; +using GeneralCache = Cache; + // Classifications of block cache entries. // // Developer notes: Adding a new enum to this class requires corresponding @@ -135,7 +155,8 @@ struct ShardedCacheOptions { CacheMetadataChargePolicy metadata_charge_policy = kDefaultCacheMetadataChargePolicy; - // A SecondaryCache instance to use the non-volatile tier. + // A SecondaryCache instance to use the non-volatile tier. For a GeneralCache + // this option must be kept as default empty. std::shared_ptr secondary_cache; // See hash_seed comments below @@ -236,6 +257,10 @@ struct LRUCacheOptions : public ShardedCacheOptions { // Construct an instance of LRUCache using these options std::shared_ptr MakeSharedCache() const; + + // Construct an instance of LRUCache for use as a general cache (e.g. for + // row_cache). Some options are not relevant to general caches. + std::shared_ptr MakeSharedGeneralCache() const; }; // DEPRECATED wrapper function diff --git a/include/rocksdb/options.h b/include/rocksdb/options.h index 82fee18bf..e9a708aa3 100644 --- a/include/rocksdb/options.h +++ b/include/rocksdb/options.h @@ -1157,7 +1157,7 @@ struct DBOptions { // A global cache for table-level rows. // Default: nullptr (disabled) - std::shared_ptr row_cache = nullptr; + std::shared_ptr row_cache = nullptr; // A filter object supplied to be invoked while processing write-ahead-logs // (WALs) during recovery. The filter provides a way to inspect log