Make Cache into a Customizable Object #8998

mrambacher · 2021-10-08T12:22:50Z

Make Cache into a Customizable Object and add appropriate methods
Add ability to configure cache via options
Add ability to create a cache from a URI to db_bench and cache_bench

zhichao-cao · 2021-11-02T00:30:52Z

cache/lru_cache.cc

@@ -741,7 +787,7 @@ void LRUCache::WaitAll(std::vector<Handle*>& handles) {
      }
      sec_handles.emplace_back(lru_handle->sec_handle);
    }
-    secondary_cache_->WaitAll(sec_handles);
+    options_.secondary_cache->WaitAll(sec_handles);


I think there are more places that we use "secondary_cache_->" for lookup and insertion.

Those are done within the shard, which has a secondary_cache_ element.

zhichao-cao · 2021-11-02T00:33:03Z

cache/lru_cache.h

@@ -474,8 +473,8 @@ class LRUCache

 private:
  LRUCacheShard* shards_ = nullptr;
+  LRUCacheOptions options_;


I may prefer to keep the secondary_cache_ pointer here. using options_.secondary_cache is hard to understand and maybe in the future, we can replace the secondary_cache_ on the fly (e.g., from local secondary to the remote secondary)

Whether or not it is in the LRUCacheOptions or an independent argument, the rules would be the same for what you are suggesting. As an LRUCacheOption member, it is easier to keep it with the rest of the options and treat it the same as everything else.

Since the place to pass in the SecondaryCache might change, we may keep using options_.secondary_cache for now.

zhichao-cao · 2021-11-02T00:48:55Z

include/rocksdb/cache.h

@@ -47,7 +48,19 @@ enum CacheMetadataChargePolicy {
 const CacheMetadataChargePolicy kDefaultCacheMetadataChargePolicy =
    kFullChargeCacheMetadata;

-struct LRUCacheOptions {
+struct CacheOptions {


I'm not sure, this CacheOptions is general enough. Maybe some other cache will not use shard? May need some discussion here.

I agree that at least some of these options are implementation-specific. After #9126 I'm likely to work on a new clock cache that does not use sharding, and might not support strict_capacity_limit.

Removed the CacheOptions

zhichao-cao · 2021-11-05T01:40:37Z

include/rocksdb/cache.h

+  // Caveat: when the cache is used as block cache, the memory allocator is
+  // ignored when dealing with compression libraries that allocate memory
+  // internally (currently only XPRESS).
+  std::shared_ptr<MemoryAllocator> memory_allocator;


What's the reason to move it into the CacheOptions structure? Do we pair the memory_allocator with a certain option?

It was already in the LRUCacheOptions.

zhichao-cao · 2021-12-06T19:20:24Z

cache/sharded_cache.cc

-    GetShard(s)->SetStrictCapacityLimit(strict_capacity_limit);
+Status ShardedCache::ValidateOptions(const DBOptions& db_opts,
+                                     const ColumnFamilyOptions& cf_opts) const {
+  if (IsMutable()) {


Why we need "IsMutable()" as a member function for cache? It is unclear to me what it means: the size is mutable? or the configuration is mutable?

zhichao-cao · 2021-12-06T19:28:17Z

cache/lru_cache.cc

+    options_.num_shard_bits = GetDefaultCacheShardBits(options_.capacity);
+  }
+  Status s = ShardedCache::PrepareOptions(config_options);
+  if (s.ok()) {


Start from here, I do not think it belongs to the function of "PrepareOptions". We may need another function for initiation and call it after "PrepareOptions" in "NewLRUCache"

zhichao-cao · 2021-12-06T19:31:48Z

cache/lru_cache.h

@@ -474,8 +482,7 @@ class LRUCache

 private:
  LRUCacheShard* shards_ = nullptr;
-  int num_shards_ = 0;


Why we remove this member?

zhichao-cao · 2021-12-06T19:35:10Z

cache/clock_cache.cc

+        options_.num_shard_bits = GetDefaultCacheShardBits(options_.capacity);
+      }
+      s = ShardedCache::PrepareOptions(config_options);
+      if (s.ok()) {


It is the same here. I prefer not to put them in "PrepareOptions"

zhichao-cao · 2021-12-06T19:38:44Z

include/rocksdb/cache.h

@@ -384,9 +387,7 @@ class Cache {
  // Prerequisite: no entry is referenced.
  virtual void EraseUnRefEntries() = 0;

-  virtual std::string GetPrintableOptions() const { return ""; }


Can we keep this interface even we can call Customizable function to achieve the same?

It is already there (via Configurable)

zhichao-cao · 2021-12-06T19:49:47Z

include/rocksdb/cache.h

@@ -209,12 +213,12 @@ class Cache {
  using CreateCallback = std::function<Status(void* buf, size_t size,
                                              void** out_obj, size_t* charge)>;

-  Cache(std::shared_ptr<MemoryAllocator> allocator = nullptr)


Move the memory_allocator out of the abstract class "Cache" and use it as a member of cache option looks good to me. But one thing I'm not sure is, if it will make some interface compatibility issues for some users how developed their own cache?

It goes both ways. If there is a non-standard Cache that was expecting/relying on the memory allocator, then this will change that class. OTOH, if there is a cache that does not use the RocksDB allocators (like potentially CacheLib), the implementation would not have to provide one.

facebook-github-bot · 2021-12-09T17:22:15Z

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

Allow db_bench and cache_bench to create Cache from URI.

facebook-github-bot · 2022-05-27T15:32:05Z

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

mrambacher added 12 commits July 30, 2021 14:58

First pass at making Cache into Customizable

15eb003

Fix ClockCache for Customizable; fix LITE mode

cf8cd5a

Merge branch 'master' into CustomCache

93faa3a

Revert MemoryAllocator changes

229ab4a

Merge branch 'master' into CustomCache

ce46b19

Fix LITE mode

405f0bc

Update to latest

0d71eb2

Merge branch 'main' into CustomCache

a7148d6

Make Cache into a Customizable, Managed Object

c876533

Fix build/test issues

601ae25

make format

247f32d

Another format...

5652289

facebook-github-bot added the CLA Signed label Oct 13, 2021

zhichao-cao reviewed Nov 2, 2021

View reviewed changes

zhichao-cao reviewed Nov 5, 2021

View reviewed changes

mrambacher added 9 commits November 8, 2021 15:22

Update to latest

0cc22db

Remove backup files

cf14565

Address PR Comments

35cac19

Fix Linux build issue

b656948

make format

52cc40a

Add more using statements for Linux

dfc4013

Attempt to fix build issue

bc3941d

Fix unity test failure

cad0cf9

Merge branch 'main' into CustomCache

d3cbd92

zhichao-cao reviewed Dec 6, 2021

View reviewed changes

mrambacher added 3 commits May 26, 2022 17:30

Merge branch 'main' into CustomCache

a22f56f

Fix merge

2b006be

Add FastLRUCache to the ObjectRegistry

6c6815a

Allow db_bench and cache_bench to create Cache from URI.

mrambacher mentioned this pull request May 31, 2022

Add support for FastLRUCache in stress and crash tests. #10081

Closed

mrambacher changed the title ~~Make Cache into a Customizable Managed Object~~ Make Cache into a Customizable Object May 31, 2022

mrambacher mentioned this pull request May 31, 2022

Create Cache as a Customizable Object #10084

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Cache into a Customizable Object #8998

Make Cache into a Customizable Object #8998

mrambacher commented Oct 8, 2021 •

edited

Loading

zhichao-cao Nov 2, 2021

mrambacher Nov 9, 2021

zhichao-cao Nov 2, 2021

mrambacher Nov 9, 2021

zhichao-cao Dec 6, 2021

zhichao-cao Nov 2, 2021

pdillinger Nov 4, 2021

mrambacher Nov 9, 2021

zhichao-cao Nov 5, 2021

mrambacher Nov 9, 2021

zhichao-cao Dec 6, 2021

zhichao-cao Dec 6, 2021

zhichao-cao Dec 6, 2021

zhichao-cao Dec 6, 2021

zhichao-cao Dec 6, 2021

mrambacher Dec 9, 2021

zhichao-cao Dec 6, 2021

mrambacher Dec 9, 2021

facebook-github-bot commented Dec 9, 2021

facebook-github-bot commented May 27, 2022

Make Cache into a Customizable Object #8998

Are you sure you want to change the base?

Make Cache into a Customizable Object #8998

Conversation

mrambacher commented Oct 8, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Dec 9, 2021

facebook-github-bot commented May 27, 2022

mrambacher commented Oct 8, 2021 •

edited

Loading