add prioritized cache wrapper #142

haoxiang47 · 2020-02-21T14:57:49Z

add a prioritized cache wrapper, for this issue: #127

haoxiang47 · 2020-02-21T15:21:09Z

@yiwu-arbug sorry for a little late, I have update this PR to add a wrapper called PrioritizedCache, has two function HighPriCacheInsert and LowPriCacheInsert which can insert high and low cache priorized regardless of user provided option, and a interface GetCache to return the Cache ptr

codecov-io · 2020-02-23T09:48:01Z

Codecov Report

Merging #142 into master will increase coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #142      +/-   ##
==========================================
+ Coverage   85.49%   85.52%   +0.02%     
==========================================
  Files          45       45              
  Lines        3419     3419              
==========================================
+ Hits         2923     2924       +1     
+ Misses        496      495       -1

yiwu-arbug

Sorry for late review.

yiwu-arbug · 2020-02-25T19:50:35Z

src/prioritized_cache.h

@@ -0,0 +1,41 @@
+#pragma once


Please move this header to include/titan/

yiwu-arbug · 2020-02-25T19:51:09Z

src/prioritized_cache.h

@@ -0,0 +1,41 @@
+#pragma once
+#include "rocksdb/cache.h"
+#include "rocksdb/env.h"


nit: env.h is not needed.

yiwu-arbug · 2020-02-25T20:04:34Z

src/prioritized_cache.h

+namespace rocksdb {
+namespace titandb {
+
+class PrioritizedCache {


Sorry I didn't explain clearly again. What I want is something like this. Do you mind update the code? Thanks.

class PrioritizedCache: public Cache { public: PrioritizedCache(std::shared_ptr<Cache> cache, Cache::Priority priority) : cache_(cache), priority_(priority) {} Status Insert(..., Cache::Priority /*priority*/) override { return Insert(..., priority_); } private: std::shared_ptr<Cache> cache_; Cache::Priority priority_; };

With it we can use the same underlying LRUCache for both rocksdb block_cache and titan blob_cache, but forcing block_cache to always insert with high-pri, and titan always insert with low-pri. This is because the data in rocksdb is the index for titan values, so they are more valuable and we want them to in cache more.

Oh, I got it, I will update the code soon.

haoxiang47 · 2020-02-29T09:35:49Z

@yiwu-arbug I have updated the code, please review again

yiwu-arbug · 2020-03-02T01:00:52Z

include/titan/prioritized_cache.h

+namespace rocksdb {
+namespace titandb {
+
+class PrioritizedCache : Cache {


s/Cache/public Cache/

and implement all other methods in Cache interface, by passing arguments directly to underlying cache_.

ok, I have updated again now, please review again @yiwu-arbug

I think you need to implement all methods (Lookup, Release, Erase, etc) in the Cache interface.

src/prioritized_cache.cc

yiwu-arbug · 2020-03-02T23:24:57Z

Mind adding some very simple unit test? Like have a mock underlying Cache object that checks what's the priority pass to it on Insert.

Signed-off-by: haoxiang47 <[email protected]>

yiwu-arbug

Great! The test is also great, but I leave some comments there.

yiwu-arbug · 2020-03-24T17:56:09Z

src/prioritized_cache_test.cc

+
+  // here we set insert Priority to high, to check if it will 
+  // insert to low in fact
+  auto lo_ok = low_cache.Insert(low_cache_key, (void*)&cacahe_value, cache->GetCapacity(), 


You can just set charge=1 instead of cache->GetCapacity(). Same below.

yiwu-arbug · 2020-03-24T17:58:12Z

src/prioritized_cache_test.cc

+    auto v = reinterpret_cast<Slice*>(cache->Value(low_handle));
+    ASSERT_EQ(cacahe_value.data(), v->data());
+  }
+


We should verify the low pri item getting evicted after an high pri item being inserted.

yiwu-arbug · 2020-03-24T17:58:52Z

src/prioritized_cache_test.cc

+  Slice low_cache_key = "low_test_key";
+
+  LRUCacheOptions options;
+  options.capacity = high_cache_key.size() + cacahe_value.size();


Let set capacity=1 for this test. See the other comments.

yiwu-arbug · 2020-03-24T18:00:19Z

src/prioritized_cache_test.cc

+  ASSERT_TRUE(hi_ok.ok());
+
+  auto high_handle = cache->Lookup(high_cache_key);
+  if (high_handle) {


Assert high_handle != nullptr.

yiwu-arbug · 2020-03-24T18:00:31Z

src/prioritized_cache_test.cc

+    ASSERT_EQ(cacahe_value.data(), v->data());
+  }
+
+  auto low_handle = cache->Lookup(low_cache_key);


assert low_handle == nulllptr.

yiwu-arbug · 2020-03-24T18:01:52Z

src/prioritized_cache_test.cc

+      &DeleteCacheValue<Slice>, &cache_handle, Cache::Priority::LOW);
+  ASSERT_TRUE(hi_ok.ok());
+
+  auto high_handle = cache->Lookup(high_cache_key);


lookup through high_cache instead of cache.

yiwu-arbug · 2020-03-24T18:02:10Z

src/prioritized_cache_test.cc

+    ASSERT_EQ(cacahe_value.data(), v->data());
+  }
+
+  auto low_handle = cache->Lookup(low_cache_key);


lookup through low_cache instead of cache.

yiwu-arbug · 2020-03-24T18:02:40Z

src/prioritized_cache_test.cc

+  auto high_handle = cache->Lookup(high_cache_key);
+  if (high_handle) {
+    auto v = reinterpret_cast<Slice*>(cache->Value(high_handle));
+    ASSERT_EQ(cacahe_value.data(), v->data());


Should release the handle here, otherwise there's memory leak: high_cache->Release(high_handle). This should fix CI.

yiwu-arbug · 2020-03-24T20:11:22Z

include/titan/prioritized_cache.h

@@ -0,0 +1,117 @@
+#pragma once


Please add the header to new files:

// Copyright 2020 TiKV Project Authors. Licensed under Apache-2.0.

Connor1996 · 2024-01-31T07:58:19Z

As blob-specific cache priority is introduced, close it tikv/rocksdb#354

sre-bot added the contribution label Feb 21, 2020

haoxiang47 force-pushed the add-prioritized-cache branch 2 times, most recently from 25fc0ff to c4ad7f0 Compare February 21, 2020 15:02

haoxiang47 force-pushed the add-prioritized-cache branch 2 times, most recently from 56fa43f to 3798713 Compare February 23, 2020 09:28

haoxiang47 requested a review from yiwu-arbug February 24, 2020 03:57

yiwu-arbug reviewed Feb 25, 2020

View reviewed changes

haoxiang47 force-pushed the add-prioritized-cache branch 2 times, most recently from 0825993 to f420771 Compare February 29, 2020 09:34

haoxiang47 force-pushed the add-prioritized-cache branch 2 times, most recently from 0eba4d4 to 62093a8 Compare February 29, 2020 15:45

yiwu-arbug reviewed Mar 2, 2020

View reviewed changes

src/prioritized_cache.cc Outdated Show resolved Hide resolved

haoxiang47 force-pushed the add-prioritized-cache branch from 62093a8 to 52ad7c3 Compare March 2, 2020 15:29

haoxiang47 requested a review from yiwu-arbug March 2, 2020 16:21

add prioritized cache wrapper

199513e

Signed-off-by: haoxiang47 <[email protected]>

haoxiang47 force-pushed the add-prioritized-cache branch from 52ad7c3 to 199513e Compare March 24, 2020 15:22

yiwu-arbug reviewed Mar 24, 2020

View reviewed changes

yiwu-arbug mentioned this pull request Mar 24, 2020

add HighPriCache and LowPriCache interface to implement PrioritizedCache tikv/rocksdb#144

Closed

Connor1996 closed this Jan 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add prioritized cache wrapper #142

add prioritized cache wrapper #142

haoxiang47 commented Feb 21, 2020

haoxiang47 commented Feb 21, 2020

codecov-io commented Feb 23, 2020 •

edited

Loading

yiwu-arbug left a comment

yiwu-arbug Feb 25, 2020

yiwu-arbug Feb 25, 2020

yiwu-arbug Feb 25, 2020

yiwu-arbug Feb 25, 2020

haoxiang47 Feb 26, 2020

haoxiang47 commented Feb 29, 2020

yiwu-arbug Mar 2, 2020

haoxiang47 Mar 2, 2020

yiwu-arbug Mar 2, 2020

yiwu-arbug commented Mar 2, 2020

yiwu-arbug left a comment

yiwu-arbug Mar 24, 2020

yiwu-arbug Mar 24, 2020

yiwu-arbug Mar 24, 2020

yiwu-arbug Mar 24, 2020

yiwu-arbug Mar 24, 2020

yiwu-arbug Mar 24, 2020

yiwu-arbug Mar 24, 2020

yiwu-arbug Mar 24, 2020

yiwu-arbug Mar 24, 2020

Connor1996 commented Jan 31, 2024

add prioritized cache wrapper #142

add prioritized cache wrapper #142

Conversation

haoxiang47 commented Feb 21, 2020

haoxiang47 commented Feb 21, 2020

codecov-io commented Feb 23, 2020 • edited Loading

Codecov Report

yiwu-arbug 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haoxiang47 commented Feb 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yiwu-arbug commented Mar 2, 2020

yiwu-arbug 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

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

Connor1996 commented Jan 31, 2024

codecov-io commented Feb 23, 2020 •

edited

Loading