Create Cache as a Customizable Object #10084

mrambacher · 2022-05-31T20:56:08Z

This is a predecessor to #8998. This change adds the ability to create a Cache from the ObjectRegistry. Unlike 8998, each call to Cache::CreateFromString will create a new object -- no managed object code is involved here.

Allow db_bench and cache_bench to create Cache from URI.

pdillinger

Rebase or merge to fix protobuf CI issue
Fix clang-analyze issue
Add to HISTORY.md
Other inline suggestions

pdillinger · 2022-06-01T16:55:57Z

cache/clock_cache.cc


-  void WaitAll(std::vector<Handle*>& /*handles*/) override {}
+CacheShard* ClockCache::GetShard(uint32_t shard) {
+#ifdef SUPPORT_CLOCK_CACHE


I don't like all this repeated ifdef, but I can accept it since we're likely getting rid of this code soon.

cache/sharded_cache.h

pdillinger · 2022-06-01T18:31:57Z

include/rocksdb/cache.h

@@ -535,9 +537,6 @@ class Cache {
  // thread safe and should only be called by the caller holding a reference
  // to each of the handles.
  virtual void WaitAll(std::vector<Handle*>& /*handles*/) {}
-
- private:
-  std::shared_ptr<MemoryAllocator> memory_allocator_;


OK by me. As long as we are clear in HISTORY.md that the API updates to Cache require some updates to custom implementations, I think it will be fine.

pdillinger · 2022-06-01T18:35:20Z

options/customizable.cc

+  } else {
+    auto ipos = id.find("@");
+    auto tpos = that_id.find("@");
+    if (ipos != std::string::npos && tpos != std::string::npos) {


Here or in API comment say that it ignores trailing @<anything>?

mrambacher · 2022-06-15T16:53:09Z

@pdillinger I believe I resolved your comments. As for the "analyze" test that is failing, I tried on an ubuntu docker container with a comparable setup (same version of clang, etc) and see no errors. Would there be any way to get the errors off the Circle box into the log or some other way to tell what is failing? TIA!

pdillinger · 2022-06-16T00:08:54Z

The errors are in the output (with enough scrolling)

cache/cache_bench_tool.cc:809:35: warning: Use of zero-allocated memory
      dbs_[db_i].orig_file_number += 1;
                                  ^
cache/cache_bench_tool.cc:900:32: warning: Use of zero-allocated memory
      dbs_[0].orig_file_number = 0;
                               ^
2 warnings generated.

pdillinger · 2022-06-16T15:39:14Z

Weird error. I thought it was talking about zero-initialized memory, but I think it means db_count might be zero. If you coerce or assert db_count >= 1 that should suffice.

facebook-github-bot · 2022-06-16T18:04:42Z

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

pdillinger

Seems to be good now

jay-zhuang

LGTM. Can you please run benchmark and share the result. Thanks.

And a few minor format issues would be nice to fix.

cache/cache.cc

jay-zhuang · 2022-06-22T22:51:01Z

cache/cache.cc

+        if (!s.ok()) {
+          *errmsg = s.ToString();
+        }
+        guard->reset(clock.release());


Nit:

Suggested change

guard->reset(clock.release());

*guard = std::move(clock);

jay-zhuang · 2022-06-22T22:52:00Z

cache/cache.cc

+        if (!s.ok()) {
+          *errmsg = s.ToString();
+        }
+        guard->reset(clock.release());


nit:

Suggested change

guard->reset(clock.release());

*guard = std::move(clock);

jay-zhuang · 2022-06-22T22:53:23Z

cache/cache.cc

+#ifndef ROCKSDB_LITE
+  static std::once_flag once;
+  std::call_once(once, [&]() {
+    RegisterBuiltinCache(*(ObjectLibrary::Default().get()), "");


nit:

Suggested change

RegisterBuiltinCache(*(ObjectLibrary::Default().get()), "");

RegisterBuiltinCache(*(ObjectLibrary::Default()), "");

jay-zhuang · 2022-06-22T22:54:51Z

cache/cache.cc

+          lru_cache::LRUCache::kClassName()),
+      [](const std::string& /*uri*/, std::unique_ptr<Cache>* guard,
+         std::string* /* errmsg */) {
+        guard->reset(new lru_cache::LRUCache());


nit:

Suggested change

guard->reset(new lru_cache::LRUCache());

*guard = std::make_unique<lru_cache::LRUCache>();

jay-zhuang · 2022-06-22T23:06:44Z

cache/clock_cache.h

+class ClockCacheShard;
+
+struct ClockCacheOptions {
+  ClockCacheOptions() {}


nit:

Suggested change

ClockCacheOptions() {}

ClockCacheOptions() = default;

jay-zhuang · 2022-06-22T23:08:36Z

cache/clock_cache.h

+  virtual std::string GetPrintableOptions() const override;
+
+  virtual void SetCapacity(size_t capacity) override;
+  virtual void SetStrictCapacityLimit(bool strict_capacity_limit) override;
+  virtual size_t GetCapacity() const override;
+  virtual bool HasStrictCapacityLimit() const override;


nit, virtual is redundant with override:

Suggested change

virtual std::string GetPrintableOptions() const override;

virtual void SetCapacity(size_t capacity) override;

virtual void SetStrictCapacityLimit(bool strict_capacity_limit) override;

virtual size_t GetCapacity() const override;

virtual bool HasStrictCapacityLimit() const override;

std::string GetPrintableOptions() const override;

void SetCapacity(size_t capacity) override;

void SetStrictCapacityLimit(bool strict_capacity_limit) override;

size_t GetCapacity() const override;

bool HasStrictCapacityLimit() const override;

jay-zhuang · 2022-06-22T23:21:27Z

options/customizable.cc

+namespace {
+// Splits an IndividualId into its three components (name@addr#pid).
+// Returns true if the input id had all components and false otherwise.
+static bool IsIndividualId(const std::string& id, size_t* addr,


nit:

Suggested change

static bool IsIndividualId(const std::string& id, size_t* addr,

bool IsIndividualId(const std::string& id, size_t* addr,

jay-zhuang · 2022-06-22T23:21:59Z

options/customizable.cc

+static bool IsIndividualId(const std::string& id, size_t* addr,
+                           size_t* pid = nullptr) {
+  auto hoff = std::string::npos;
+  *addr = id.find("@");


nit:

Suggested change

*addr = id.find("@");

*addr = id.find('@');

jay-zhuang · 2022-06-22T23:22:38Z

options/customizable.cc

+  auto hoff = std::string::npos;
+  *addr = id.find("@");
+  if (*addr != std::string::npos) {
+    hoff = id.find("#", *addr);


Suggested change

hoff = id.find("#", *addr);

hoff = id.find('#', *addr);

Merge to latest cache implementations. Fixed some issues with tests and initializations.

facebook-github-bot · 2022-07-15T18:31:20Z

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

facebook-github-bot · 2022-07-15T18:53:47Z

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

facebook-github-bot · 2022-07-15T18:58:12Z

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

facebook-github-bot · 2022-07-26T18:48:58Z

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

facebook-github-bot · 2022-07-28T18:12:43Z

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

mrambacher · 2022-07-28T20:00:58Z

@pdillinger @jay-zhuang I think I have addressed all of the PR comments. Can this be merged soon? Keeping up with the Cache changes is getting to be tricky. Thanks!

pdillinger · 2022-07-28T21:05:07Z

May have chosen a bad time to merge 🙁 (#10434 )

pdillinger · 2022-07-28T21:31:47Z

Also, we are concerned about the race conditions in customizable / setoptions infrastructure, e.g. #10079 and #10375. Plus we're cautious about other functional regressions like the one patched in #10378

mrambacher · 2022-07-28T21:56:26Z

Also, we are concerned about the race conditions in customizable / setoptions infrastructure, e.g. #10079 and #10375. Plus we're cautious about other functional regressions like the one patched in #10378

I created #10387 to eliminate many of the extra calls to PrepareOptions that were potential with #10375. In #9756, I changed code that will also eliminate some of the potential PrepareOptions calls (if bad name=value were passed to Configure), as well as improve the performance.

I do not know if these PRs will address the other race conditions, but I would think they might and are worth a look.

Also note that if Cache is not marked as Mutable, it cannot be changed via SetOptions. If the properties themselves (e.g. "capacity" are not mutable, they cannot be changed via SetOptions and only via the corresponding API. I doubt the same race condition(s) apply here.

Fix merge related compilation issue

facebook-github-bot · 2022-09-16T12:14:37Z

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

mrambacher added 25 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

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

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.

Fix Cache::CreateFromString and comparisons

b59e773

mrambacher requested a review from pdillinger May 31, 2022 20:56

facebook-github-bot added the CLA Signed label May 31, 2022

pdillinger mentioned this pull request Jun 1, 2022

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

Closed

pdillinger requested changes Jun 1, 2022

View reviewed changes

Merge branch 'main' into CreateCacheFromString

d58a7d0

make format

c3d6bd7

pdillinger added 3 commits June 16, 2022 10:31

Merge remote-tracking branch 'origin/main' into pr/10084

14509a0

Hopefully fix analyzer error

799e47a

Don't change to const std::shared_ptr<X>& if always saving a copy

cf11b94

pdillinger approved these changes Jun 16, 2022

View reviewed changes

jay-zhuang approved these changes Jun 22, 2022

View reviewed changes

mrambacher added 2 commits July 13, 2022 18:23

Merge branch 'main' into CreateCacheFromString

ddbae7d

Update to latest

6e5f5c9

Merge to latest cache implementations. Fixed some issues with tests and initializations.

Yet another make format

e91119a

mrambacher added 2 commits July 26, 2022 12:20

Merge branch 'main' into CreateCacheFromString

db1ab36

Fix test

5a2e8b8

mrambacher added 2 commits July 28, 2022 13:52

Merge branch 'main' into CreateCacheFromString

4502655

Fix merge

4ad004d

mrambacher added 2 commits September 15, 2022 17:14

Merge branch 'main' into CreateCacheFromString

23e6d33

Update lru_cache.cc

760821c

Fix merge related compilation issue

mrambacher mentioned this pull request Sep 16, 2022

Revamp, optimize new experimental clock cache #10626

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create Cache as a Customizable Object #10084

Create Cache as a Customizable Object #10084

mrambacher commented May 31, 2022

pdillinger left a comment

pdillinger Jun 1, 2022

pdillinger Jun 1, 2022

pdillinger Jun 1, 2022

mrambacher commented Jun 15, 2022

pdillinger commented Jun 16, 2022

pdillinger commented Jun 16, 2022

facebook-github-bot commented Jun 16, 2022

pdillinger left a comment

jay-zhuang left a comment •

edited

Loading

jay-zhuang Jun 22, 2022

jay-zhuang Jun 22, 2022

jay-zhuang Jun 22, 2022

jay-zhuang Jun 22, 2022

jay-zhuang Jun 22, 2022

jay-zhuang Jun 22, 2022

jay-zhuang Jun 22, 2022

jay-zhuang Jun 22, 2022

jay-zhuang Jun 22, 2022

facebook-github-bot commented Jul 15, 2022

facebook-github-bot commented Jul 15, 2022

facebook-github-bot commented Jul 15, 2022

facebook-github-bot commented Jul 26, 2022

facebook-github-bot commented Jul 28, 2022

mrambacher commented Jul 28, 2022

pdillinger commented Jul 28, 2022

pdillinger commented Jul 28, 2022

mrambacher commented Jul 28, 2022

facebook-github-bot commented Sep 16, 2022

	RegisterBuiltinCache(*(ObjectLibrary::Default().get()), "");
	RegisterBuiltinCache(*(ObjectLibrary::Default()), "");

	guard->reset(new lru_cache::LRUCache());
	*guard = std::make_unique<lru_cache::LRUCache>();

	static bool IsIndividualId(const std::string& id, size_t* addr,
	bool IsIndividualId(const std::string& id, size_t* addr,

Create Cache as a Customizable Object #10084

Are you sure you want to change the base?

Create Cache as a Customizable Object #10084

Conversation

mrambacher commented May 31, 2022

pdillinger 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

mrambacher commented Jun 15, 2022

pdillinger commented Jun 16, 2022

pdillinger commented Jun 16, 2022

facebook-github-bot commented Jun 16, 2022

pdillinger left a comment

Choose a reason for hiding this comment

jay-zhuang left a comment • 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

facebook-github-bot commented Jul 15, 2022

facebook-github-bot commented Jul 15, 2022

facebook-github-bot commented Jul 15, 2022

facebook-github-bot commented Jul 26, 2022

facebook-github-bot commented Jul 28, 2022

mrambacher commented Jul 28, 2022

pdillinger commented Jul 28, 2022

pdillinger commented Jul 28, 2022

mrambacher commented Jul 28, 2022

facebook-github-bot commented Sep 16, 2022

jay-zhuang left a comment •

edited

Loading