Avoid recompressing cold block in CompressedSecondaryCache #10527

gitbw95 · 2022-08-14T06:47:20Z

Summary:
When a block is firstly Lookup from the secondary cache, we just insert a dummy block in the primary cache (charging the actual size of the block) and don’t erase the block from the secondary cache. A standalone handle is returned from Lookup. Only if the block is hit again, we erase it from the secondary cache and add it into the primary cache.

When a block is firstly evicted from the primary cache to the secondary cache, we just insert a dummy block (size 0) in the secondary cache. When the block is evicted again, it is treated as a hot block and is inserted into the secondary cache.

Implementation Details
Add a new state of LRUHandle: The handle is never inserted into the LRUCache (both hash table and LRU list) and it doesn't experience the above three states. The entry can be freed when refs becomes 0. (refs >= 1 && in_cache == false && IS_STANDALONE == true)

The behaviors of LRUCacheShard::Lookup() are updated if the secondary_cache is CompressedSecondaryCache:

If a handle is found in primary cache:
1.1. If the handle's value is not nullptr, it is returned immediately.
1.2. If the handle's value is nullptr, this means the handle is a dummy one. For a dummy handle, if it was retrieved from secondary cache, it may still exist in secondary cache.
- 1.2.1. If no valid handle can be Lookup from secondary cache, return nullptr.
- 1.2.2. If the handle from secondary cache is valid, erase it from the secondary cache and add it into the primary cache.
If a handle is not found in primary cache:
2.1. If no valid handle can be Lookup from secondary cache, return nullptr.
2.2. If the handle from secondary cache is valid, insert a dummy block in the primary cache (charging the actual size of the block) and return a standalone handle.

The behaviors of LRUCacheShard::Promote() are updated as follows:

If e->sec_handle has value, one of the following steps can happen:
1.1. Insert a dummy handle and return a standalone handle to caller when secondary_cache_ is CompressedSecondaryCache and e is a standalone handle.
1.2. Insert the item into the primary cache and return the handle to caller.
1.3. Exception handling.
If e->sec_handle has no value, mark the item as not in cache and charge the cache as its only metadata that'll shortly be released.

The behavior of CompressedSecondaryCache::Insert() is updated:

If a block is evicted from the primary cache for the first time, a dummy item is inserted.
If a dummy item is found for a block, the block is inserted into the secondary cache.

The behavior of CompressedSecondaryCache:::Lookup() is updated:

If a handle is not found or it is a dummy item, a nullptr is returned.
If erase_handle is true, the handle is erased.

The behaviors of LRUCacheShard::Release() are adjusted for the standalone handles.

Test Plan:

stress tests.
unit tests.
CPU profiling for db_bench.

facebook-github-bot · 2022-08-16T16:49:58Z

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

gitbw95 · 2022-08-16T21:51:32Z

As discussed offline, I may do the following changes:

Remove WAS_IN_SECONDARY_CACHE from LRUHandle. Add one param into Promote() to carry this information.
Charge the capacity of secondary cache for the standalone handle.
Find a way to make sure the behavior of current secondary cache implementation (e.g. CacheLibWrapper) is intact.

facebook-github-bot · 2022-08-17T20:57:09Z

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

facebook-github-bot · 2022-08-17T22:56:41Z

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

facebook-github-bot · 2022-08-17T23:37:30Z

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

facebook-github-bot · 2022-08-18T08:03:14Z

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

facebook-github-bot · 2022-08-18T18:15:19Z

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

facebook-github-bot · 2022-08-18T18:35:31Z

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

facebook-github-bot · 2022-08-19T21:01:33Z

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

facebook-github-bot · 2022-08-19T21:03:11Z

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

facebook-github-bot · 2022-08-19T22:05:08Z

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

facebook-github-bot · 2022-08-21T07:29:10Z

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

facebook-github-bot · 2022-08-21T14:21:30Z

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

facebook-github-bot · 2022-08-21T15:35:19Z

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

facebook-github-bot · 2022-08-21T23:18:41Z

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

facebook-github-bot · 2022-08-21T23:21:43Z

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

facebook-github-bot · 2022-08-21T23:59:45Z

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

facebook-github-bot · 2022-08-22T07:16:41Z

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

facebook-github-bot · 2022-08-22T07:29:06Z

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

facebook-github-bot · 2022-08-22T07:32:27Z

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

facebook-github-bot · 2022-08-22T17:33:15Z

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

facebook-github-bot · 2022-08-22T18:17:48Z

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

facebook-github-bot · 2022-08-22T19:10:23Z

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

facebook-github-bot · 2022-08-22T22:50:09Z

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

facebook-github-bot · 2022-09-07T01:35:59Z

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

facebook-github-bot · 2022-09-07T03:36:20Z

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

facebook-github-bot · 2022-09-07T03:47:00Z

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

facebook-github-bot · 2022-09-07T07:08:32Z

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

facebook-github-bot · 2022-09-07T07:10:43Z

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

siying · 2022-09-07T16:17:25Z

cache/compressed_secondary_cache.h

@@ -44,6 +46,16 @@ class CompressedSecondaryCacheResultHandle : public SecondaryCacheResultHandle {

 // The CompressedSecondaryCache is a concrete implementation of
 // rocksdb::SecondaryCache.
+// When a block is firstly Lookup from CompressedSecondaryCache, we just


"firstly Lookup" gives me a feeling that no matter whether we find a block from compressed cache or not, we would insert a dummy block. Perhaps clarify it.

siying · 2022-09-07T16:20:22Z

cache/compressed_secondary_cache.h

+//
+// When a block is firstly evicted from the primary cache to
+// CompressedSecondaryCache, we just insert a dummy block (size 0) in
+// CompressedSecondaryCache. When the block is evicted again, it is treated as


Probably should clarify "is evicted again". Only if it is evicted again before the dummy block is evicted from the cache, we insert it.

siying · 2022-09-07T16:20:41Z

cache/compressed_secondary_cache.h

-      512,  640,  768,  896,  1024, 1280, 1536,  1792,  2048,  2560,  3072,
-      3584, 4096, 5120, 6144, 7168, 8192, 10240, 12288, 14336, 16384, 32768};
+  static constexpr std::array<uint16_t, 8> malloc_bin_sizes_{
+      128, 256, 512, 1024, 2048, 4096, 8192, 16384};


Is the change related?

The split and merge functions are not included in Lookup() and Insert() now.
I will revert this change.

siying · 2022-09-07T16:22:46Z

cache/compressed_secondary_cache.h


  Status Insert(const Slice& key, void* value,
                const Cache::CacheItemHelper* helper) override;

  std::unique_ptr<SecondaryCacheResultHandle> Lookup(
      const Slice& key, const Cache::CreateCallback& create_cb, bool /*wait*/,
-      bool& is_in_sec_cache) override;
+      bool advise_erase, bool& is_in_sec_cache) override;


Need comment here to explain the requirement for the Lookup() implementation by advise_erase.

siying · 2022-09-07T16:33:40Z

cache/lru_cache.cc

+
+        if ((usage_ + e->total_charge) > capacity_ && strict_capacity_limit_) {
+          e->Unref();
+          e->Free();


Perhaps I missed something but in my impression we don't call Free() in the mutex.

siying · 2022-09-07T16:35:02Z

cache/lru_cache.cc

+              .PermitUncheckedError();
+        }
+        entry->Free();
+      }


The whole for-loop must be copied from somewhere else. Can we define a helper function and call it once?

siying · 2022-09-07T16:50:03Z

cache/lru_cache.cc

@@ -463,34 +514,59 @@ Cache::Handle* LRUCacheShard::Lookup(
    const ShardedCache::CreateCallback& create_cb, Cache::Priority priority,
    bool wait, Statistics* stats) {
  LRUHandle* e = nullptr;
+  bool erase_handle_in_sec_cache{false};


It might be easier to understand if it is called something like "found_dummy_entry".

siying · 2022-09-07T16:58:54Z

cache/compressed_secondary_cache_test.cc

+        "k1", test_item_creator, true, /*advise_erase=*/false, is_in_sec_cache);
+    ASSERT_EQ(handle1, nullptr);
+
+    // Insert k2 and k1 is evicted.


In this case, we should also cover the case where, re-inserting k1 will insert a dummy handle, rather than a compressed block. Ignore this comment if the scenario is covered somewhere else.

siying · 2022-09-07T17:01:44Z

cache/compressed_secondary_cache_test.cc

+    // k1's item and k2's dummy item.
+    ASSERT_OK(cache->Insert(
+        "k2", item2_2, &CompressedSecondaryCacheTest::helper_, str2.length()));
+


It's great to see all those scenarios are covered in the unit test!

facebook-github-bot · 2022-09-07T20:47:19Z

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

facebook-github-bot · 2022-09-07T21:25:24Z

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

facebook-github-bot · 2022-09-07T21:54:20Z

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

facebook-github-bot · 2022-09-07T21:55:39Z

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

facebook-github-bot · 2022-09-07T22:37:39Z

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

facebook-github-bot · 2022-09-07T22:38:37Z

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

facebook-github-bot · 2022-09-07T23:48:23Z

@gitbw95 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 Aug 14, 2022

gitbw95 force-pushed the avoid_compress_cold_block_1 branch from da1ef91 to 412e4dc Compare August 14, 2022 06:55

gitbw95 changed the title ~~[Testing ]Avoid recompressing cold block~~ [WIP ]Avoid recompressing cold block in CompressedSecondaryCache Aug 16, 2022

gitbw95 marked this pull request as ready for review August 16, 2022 15:53

gitbw95 requested a review from anand1976 August 16, 2022 15:53

gitbw95 changed the title ~~[WIP ]Avoid recompressing cold block in CompressedSecondaryCache~~ Avoid recompressing cold block in CompressedSecondaryCache Aug 22, 2022

bowang added 7 commits September 6, 2022 18:34

update malloc_bin_sizes_.

d1621ab

fix unit tests.

17f2eaa

avoid split and merge.

cf9c869

add HISTORY.md

0b136a7

address the comments.

778717f

fix a crash issue.

22170f3

merge main.

92e5a50

gitbw95 force-pushed the avoid_compress_cold_block_1 branch from e872961 to 92e5a50 Compare September 7, 2022 01:35

update

b6543b2

rename erase_handle to advise_erase.

4ed4448

siying reviewed Sep 7, 2022

View reviewed changes

address comments.

f7dca22

siying approved these changes Sep 7, 2022

View reviewed changes

fix a character issue.

d39ae15

address a comment.

3c82ce3

facebook-github-bot closed this in d490bfc Sep 8, 2022

Avoid recompressing cold block in CompressedSecondaryCache #10527

Avoid recompressing cold block in CompressedSecondaryCache #10527

Conversation

gitbw95 commented Aug 14, 2022 • edited Loading

facebook-github-bot commented Aug 16, 2022

gitbw95 commented Aug 16, 2022

facebook-github-bot commented Aug 17, 2022

facebook-github-bot commented Aug 17, 2022

facebook-github-bot commented Aug 17, 2022

facebook-github-bot commented Aug 18, 2022

facebook-github-bot commented Aug 18, 2022

facebook-github-bot commented Aug 18, 2022

facebook-github-bot commented Aug 19, 2022

facebook-github-bot commented Aug 19, 2022

facebook-github-bot commented Aug 19, 2022

facebook-github-bot commented Aug 21, 2022

facebook-github-bot commented Aug 21, 2022

facebook-github-bot commented Aug 21, 2022

facebook-github-bot commented Aug 21, 2022

facebook-github-bot commented Aug 21, 2022

facebook-github-bot commented Aug 21, 2022

facebook-github-bot commented Aug 22, 2022

facebook-github-bot commented Aug 22, 2022

facebook-github-bot commented Aug 22, 2022

facebook-github-bot commented Aug 22, 2022

facebook-github-bot commented Aug 22, 2022

facebook-github-bot commented Aug 22, 2022

facebook-github-bot commented Aug 22, 2022

facebook-github-bot commented Sep 7, 2022

facebook-github-bot commented Sep 7, 2022

facebook-github-bot commented Sep 7, 2022

facebook-github-bot commented Sep 7, 2022

facebook-github-bot commented Sep 7, 2022

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 Sep 7, 2022

facebook-github-bot commented Sep 7, 2022

facebook-github-bot commented Sep 7, 2022

facebook-github-bot commented Sep 7, 2022

facebook-github-bot commented Sep 7, 2022

facebook-github-bot commented Sep 7, 2022

facebook-github-bot commented Sep 7, 2022

gitbw95 commented Aug 14, 2022 •

edited

Loading