Charge blob cache usage against the global memory limit #10321

gangliao · 2022-07-07T09:18:35Z

Summary:

To help service owners to manage their memory budget effectively, we have been working towards counting all major memory users inside RocksDB towards a single global memory limit (see e.g. https://github.com/facebook/rocksdb/wiki/Write-Buffer-Manager#cost-memory-used-in-memtable-to-block-cache). The global limit is specified by the capacity of the block-based table's block cache, and is technically implemented by inserting dummy entries ("reservations") into the block cache. The goal of this task is to support charging the memory usage of the new blob cache against this global memory limit when the backing cache of the blob cache and the block cache are different.

This PR is a part of #10156

include/rocksdb/cache.h

gangliao · 2022-07-07T18:06:21Z

If we can charge blob cache usage against the global memory limit, we can also do that for blob file cache. But it's no particular urgency since the blob file cache is pretty small.

ltamasi

Thanks for the PR @gangliao !

ltamasi · 2022-07-13T10:38:00Z

cache/sharded_cache.cc

+  Status s = GetShard(Shard(hash))
+                 ->Insert(key, hash, value, charge, deleter, handle, priority);
+  if (s.ok() && cache_res_mgr_) {
+    // Insert may cause the cache entry eviction if the cache is full. So we
+    // directly call the reservation manager to update the total memory used
+    // in the cache.
+    cache_res_mgr_->UpdateCacheReservation(GetUsage()).PermitUncheckedError();
+  }
+  return s;


Instead of adding the cache reservation manager directly to ShardedCache, we could consider moving this logic a separate class which also implements the Cache interface, wraps another Cache, takes care of the cache reservations and forwards calls to the wrapped cache. This would be a more decoupled design, and would also make the charging logic work with any custom cache implementations.

We might want to add some validation to ensure that such a "cache reservation aware" cache is configured as a blob cache if and only if charging is enabled for CacheEntryRole::kBlobCache.

if and only if charging is enabled for CacheEntryRole::kBlobCache.

If we do open up API usage like https://github.com/facebook/rocksdb/pull/10321/files#diff-5c4ced6afb6a90e27fec18ab03b2cd89e8f99db87791b4ecc6fa2694284d50c0R2921-R2925 to users, update to BlockBasedTableOptions::cache_usage_options API comment is also needed.

Also, should we consider early option validation like https://github.com/facebook/rocksdb/blob/7.4.fb/table/block_based/block_based_table_factory.cc#L709-L714 but for kBlobCache e.g, no blob cache is specified or blob cache == block cache when CacheEntryRole::kBlobCache is enabled?

Instead of adding the cache reservation manager directly to ShardedCache, we could consider moving this logic a separate class which also implements the Cache interface, wraps another Cache, takes care of the cache reservations and forwards calls to the wrapped cache. This would be a more decoupled design, and would also make the charging logic work with any custom cache implementations.

Where is this cache wrapper created? It looks like the user can create it and pass it to options.blob_cache?
In this case, it's hard to add some validation. The way @hx235 said above is feasible, even if the options.blob_cache settings here are wrong, we are able to detect anomalies from option validation in block_based_table_factory and throw an error.

Also, should we consider early option validation like https://github.com/facebook/rocksdb/blob/7.4.fb/table/block_based/block_based_table_factory.cc#L709-L714 but for kBlobCache e.g, no blob cache is specified or blob cache == block cache when CacheEntryRole::kBlobCache is enabled?

Yes, and I suppose we could also do the validation I mentioned here (i.e. make sure that options.blob_cache is the above-mentioned special type of cache iff the blob cache is charged to the block cache).

Where is this cache wrapper created? It looks like the user can create it and pass it to options.blob_cache? In this case, it's hard to add some validation. The way @hx235 said above is feasible, even if the options.blob_cache settings here are wrong, we are able to detect anomalies from option validation in block_based_table_factory and throw an error.

Yes, it would be created by the application and assigned to options.blob_cache. During validation, we could use e.g. dynamic_cast to ensure the application configured the right type of cache.

Looks like dynamic_cast is not allowed in rocksdb. I used a different approach here.

Sorry, I'm kind of having second thoughts re: having the application create the wrapper. I'm wondering if it would be better if we created it e.g. in BlobSource ? I feel it would be more user-friendly and would hide this implementation detail. The downside of doing so would be that the book-keeping would be bypassed if the application manipulates the underlying cache directly but that's not something they should be doing anyways...

No problem, I also wanted to create a wrapper in the blob source at first. But if we create the cache wrapper in the blob source, how to ensure that rocksdb will call the cache wrapper's release or erase. I expect this eviction will happen automatically.

Maybe I'm missing something but I think if we turned BlobSource::blob_cache_ into the wrapper, it would work pretty much out of the box. BlobSource would be interacting with the wrapper, and when we transfer the cache handle to PinnableSlice, it would carry the address of the wrapper cache, no?

db/blob/blob_source_test.cc

include/rocksdb/cache.h

db/blob/blob_source_test.cc

db/blob/blob_source.h

hx235

I briefly recalled from our last discussion that (1) we will not make change to sharded_cache level since it's mainly for cache sharing and (2) we will not make change to our public interface cache.h

Any further discussion that I am not part of on why (1) and (2) are both violated in this PR?

The level of abstraction we looked at was lru_cache.h although it might suffer from being limited to only lru cache implementation (which was fine to me)

But I like what @ltamasi has mentioned about the wrapper idea above - which may even overcome this limitation.

gangliao · 2022-07-14T00:34:15Z

I briefly recalled from our last discussion that (1) we will not make change to sharded_cache level since it's mainly for cache sharing and (2) we will not make change to our public interface cache.h Any further discussion that I am not part of on why (1) and (2) are both violated in this PR?

This is because we only have Cache* type for blob cache. dynamic_cast it to lru cache in blob source is not very general for long term goal. So here I use virtual function polymorphism.

The level of abstraction we looked at was lru_cache.h although it might suffer from being limited to only lru cache implementation.

class LRUCache
#ifdef NDEBUG
    final
#endif
    : public ShardedCache {

LRUCache does not overload those functions: Insert, Release, Erase,...

The real impl is LRUCacheShard final : public CacheShard, but CacheShard is not ShardedCache. Yes, the class names around here are confusing.

tools/db_bench_tool.cc

gangliao · 2022-07-14T00:44:21Z

I like what @ltamasi has mentioned about the wrapper idea above - which may solve this limitation.

I agree, that's a good idea!

hx235 · 2022-07-14T01:19:51Z

This is because we only have Cache* type for blob cache.

Oh okay - then it seems wrapper approach will saves us from needing to set CRM directly and change public interface.

facebook-github-bot · 2022-07-17T16:04:56Z

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

facebook-github-bot · 2022-07-18T06:57:57Z

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

ltamasi

Thanks for the updates @gangliao !! Looks pretty good! Have some mostly minor comments, please see below

HISTORY.md

cache/charged_cache.h

cache/charged_cache.cc

cache/charged_cache.h

db/blob/blob_source_test.cc

include/rocksdb/cache.h

table/block_based/block_based_table_factory.cc

tools/db_bench_tool.cc

db_stress_tool/db_stress_test_base.cc

facebook-github-bot · 2022-07-18T15:43:25Z

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

cache/charged_cache.cc

facebook-github-bot · 2022-07-19T03:36:19Z

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

facebook-github-bot · 2022-07-19T03:36:46Z

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

facebook-github-bot · 2022-07-19T04:19:18Z

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

facebook-github-bot · 2022-07-19T04:19:32Z

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

facebook-github-bot added the CLA Signed label Jul 7, 2022

gangliao commented Jul 7, 2022

View reviewed changes

include/rocksdb/cache.h Outdated Show resolved Hide resolved

gangliao requested review from hx235, ltamasi, riversand963 and akankshamahajan15 July 7, 2022 22:52

gangliao mentioned this pull request Jul 8, 2022

BlobDB Caching #10156

Closed

14 tasks

ltamasi reviewed Jul 13, 2022

View reviewed changes

include/rocksdb/cache.h Show resolved Hide resolved

hx235 mentioned this pull request Jul 13, 2022

Charge blob cache usage against the global memory limit #10206

Closed

hx235 reviewed Jul 13, 2022

View reviewed changes

db/blob/blob_source_test.cc Show resolved Hide resolved

hx235 reviewed Jul 13, 2022

View reviewed changes

db/blob/blob_source_test.cc Outdated Show resolved Hide resolved

hx235 reviewed Jul 13, 2022

View reviewed changes

db/blob/blob_source.h Outdated Show resolved Hide resolved

hx235 self-requested a review July 14, 2022 00:12

hx235 requested changes Jul 14, 2022

View reviewed changes

hx235 reviewed Jul 14, 2022

View reviewed changes

tools/db_bench_tool.cc Show resolved Hide resolved

gangliao force-pushed the new_global_limit branch 2 times, most recently from 8618c17 to 558a677 Compare July 14, 2022 21:53

gangliao requested a review from hx235 July 15, 2022 00:07

gangliao force-pushed the new_global_limit branch from 3301840 to 83bdd8d Compare July 17, 2022 15:55

ltamasi reviewed Jul 18, 2022

View reviewed changes

ltamasi approved these changes Jul 18, 2022

View reviewed changes

hx235 reviewed Jul 18, 2022

View reviewed changes

cache/charged_cache.cc Outdated Show resolved Hide resolved

gangliao added 22 commits July 18, 2022 20:34

Cleanup

8aff6e6

Cleanup

57266f6

Move cache_res_mgr to shared_cache

369043f

Use virtual funcs and override it in sharded_cache

0f9af0c

Fix status check

9964c2b

Check cache usage after erasing entries

04e1373

add BlobSourceCacheReservationTest

6b471b0

Add gflag charge_blob_cache in db bench and db stress

be3e195

Fix typo

9c51614

Follow the comments

f7664d6

Follow the comment

a58c1e9

Follow the comments

8ad8d9c

Follow the comments

5d4f3c8

Compare the cache name

58c464a

Add the description of charged_cache

c9423ae

Cleanup

22d06fc

Add cache wrapper in blob source

1d291f2

use datasource's GetBlobCache

623f63b

Fix a error

62586f8

Fix Linter error

ce524c3

Follow the comments

2e28dea

follow the comments

6938052

gangliao force-pushed the new_global_limit branch from fab20dc to 6938052 Compare July 19, 2022 03:36

Follow the comment

4f36fca

facebook-github-bot closed this in 0b6bc10 Jul 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Charge blob cache usage against the global memory limit #10321

Charge blob cache usage against the global memory limit #10321

gangliao commented Jul 7, 2022

gangliao commented Jul 7, 2022

ltamasi left a comment

ltamasi Jul 13, 2022

ltamasi Jul 13, 2022

hx235 Jul 14, 2022 •

edited

Loading

gangliao Jul 14, 2022 •

edited

Loading

ltamasi Jul 14, 2022

ltamasi Jul 14, 2022

gangliao Jul 14, 2022 •

edited

Loading

ltamasi Jul 15, 2022

gangliao Jul 15, 2022 •

edited

Loading

ltamasi Jul 15, 2022

hx235 left a comment •

edited

Loading

gangliao commented Jul 14, 2022 •

edited

Loading

gangliao commented Jul 14, 2022

hx235 commented Jul 14, 2022

facebook-github-bot commented Jul 17, 2022

facebook-github-bot commented Jul 18, 2022

ltamasi left a comment

facebook-github-bot commented Jul 18, 2022

facebook-github-bot commented Jul 19, 2022

facebook-github-bot commented Jul 19, 2022

facebook-github-bot commented Jul 19, 2022

facebook-github-bot commented Jul 19, 2022

Charge blob cache usage against the global memory limit #10321

Charge blob cache usage against the global memory limit #10321

Conversation

gangliao commented Jul 7, 2022

gangliao commented Jul 7, 2022

ltamasi left a comment

Choose a reason for hiding this comment

ltamasi Jul 13, 2022

Choose a reason for hiding this comment

ltamasi Jul 13, 2022

Choose a reason for hiding this comment

hx235 Jul 14, 2022 • edited Loading

Choose a reason for hiding this comment

gangliao Jul 14, 2022 • edited Loading

Choose a reason for hiding this comment

ltamasi Jul 14, 2022

Choose a reason for hiding this comment

ltamasi Jul 14, 2022

Choose a reason for hiding this comment

gangliao Jul 14, 2022 • edited Loading

Choose a reason for hiding this comment

ltamasi Jul 15, 2022

Choose a reason for hiding this comment

gangliao Jul 15, 2022 • edited Loading

Choose a reason for hiding this comment

ltamasi Jul 15, 2022

Choose a reason for hiding this comment

hx235 left a comment • edited Loading

Choose a reason for hiding this comment

gangliao commented Jul 14, 2022 • edited Loading

gangliao commented Jul 14, 2022

hx235 commented Jul 14, 2022

facebook-github-bot commented Jul 17, 2022

facebook-github-bot commented Jul 18, 2022

ltamasi left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Jul 18, 2022

facebook-github-bot commented Jul 19, 2022

facebook-github-bot commented Jul 19, 2022

facebook-github-bot commented Jul 19, 2022

facebook-github-bot commented Jul 19, 2022

hx235 Jul 14, 2022 •

edited

Loading

gangliao Jul 14, 2022 •

edited

Loading

gangliao Jul 14, 2022 •

edited

Loading

gangliao Jul 15, 2022 •

edited

Loading

hx235 left a comment •

edited

Loading

gangliao commented Jul 14, 2022 •

edited

Loading