enable PinnableSlice for RowCache #2492

sushmaad · 2017-06-24T00:17:37Z

This patch enables using PinnableSlice for RowCache, changes include
not releasing the cache handle immediately after lookup in TableCache::Get, instead pass a Cleanble function which does Cache::RleaseHandle.

facebook-github-bot · 2017-06-24T00:17:44Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

facebook-github-bot · 2017-06-24T00:28:50Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

facebook-github-bot · 2017-06-24T00:30:19Z

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

maysamyabandeh · 2017-06-26T17:03:00Z

table/get_context.cc

@@ -182,7 +182,7 @@ bool GetContext::SaveValue(const ParsedInternalKey& parsed_key,
 }

 void replayGetContextLog(const Slice& replay_log, const Slice& user_key,
-                         GetContext* get_context) {
+                         GetContext* get_context, Cleanable* val_pinner_clean) {


To be consistent with the existing code, lets call it value_pinner. Lets rename it on other places too.

maysamyabandeh

LGTM. Looking forward to a unit test.

maysamyabandeh · 2017-06-26T17:26:52Z

db/table_cache.cc

@@ -336,10 +337,17 @@ Status TableCache::Get(const ReadOptions& options,

    if (auto row_handle =
            ioptions_.row_cache->Lookup(row_cache_key.GetUserKey())) {
+
+      Cleanable cache_entry_clean;
+      // Cleanable routine to release the cache entry


Lets put the comment before the definition.

maysamyabandeh · 2017-06-26T17:29:59Z

db/table_cache.cc

      auto found_row_cache_entry = static_cast<const std::string*>(
          ioptions_.row_cache->Value(row_handle));
-      replayGetContextLog(*found_row_cache_entry, user_key, get_context);
-      ioptions_.row_cache->Release(row_handle);
+      cache_entry_clean.RegisterCleanup(release_cache_entry_func,


Add a comment to explain that the cleanup function will be delegated to PinnableSlice if the value is found, and otherwise will be invoked when this object is destructed in this code block.

ot · 2017-06-26T17:39:50Z

I'll review this today, but I just wanted to point out that there's a typo in the title, should be PinnableSlice, not PinnacleSlice :)

siying · 2017-06-26T17:49:10Z

@ot no wonder I thought of pineapple when I initially saw this PR.

ot

Thanks! This also good to me as well, after the style changes are applied.

ot · 2017-06-28T03:53:41Z

table/get_context.h

@@ -88,6 +88,7 @@ class GetContext {
 };

 void replayGetContextLog(const Slice& replay_log, const Slice& user_key,
-                         GetContext* get_context);
+                         GetContext* get_context,
+                         Cleanable *val_pinner_clean = nullptr);


Convention in the rest of the code is to juxtapose the *to the type

facebook-github-bot · 2017-06-29T22:27:39Z

@sushmaad updated the pull request - view changes - changes since last import

facebook-github-bot · 2017-06-29T22:57:08Z

@sushmaad updated the pull request - view changes - changes since last import

maysamyabandeh · 2017-06-30T23:32:23Z

db/db_test.cc

+    PinnableSlice value;
+    ASSERT_EQ(Get("foo", &value), "bar");
+    // Entry is already in cache, lookup will remove the element from lru
+    ASSERT_EQ(reinterpret_cast<LRUCache*>(options.row_cache.get())->TEST_GetLRUSize(), 0);


can you make sure to run "make format"?

maysamyabandeh · 2017-06-30T23:32:57Z

db/db_test_util.cc

+  if (s.IsNotFound()) {
+    result = "NOT_FOUND";
+  } else if (!s.ok()) {
+    result = s.ToString();


What is the motivation for mixing status and result value?

Just to keep the testcase format similar to RowCache Test

Do you think we can have them not mixed? The convention in the code base is for the return value to be Status.

test cases for Get calls in db_test.cc are validated using ASSERT_EQ statements, which checks if the return value is same as the value expected. So result of DBTestBase::Get() returns actual value in case of success and returns status code strings incase of failure. If the code is confusing and looks mixed up, I can change it to
{
.
.
Status s = dbfull()->Get(options, dbfull()->DefaultColumnFamily(), k, v);
std::string result(v->data());
if (s.IsNotFound()) {
result = "NOT_FOUND";
} else if (!s.ok()) {
result = s.ToString();
}
return result;
}

which is exactly same as which is exactly same as std::string DBTestBase::Get(int cf, const std::string& k,
const Snapshot* snapshot) in db_test_util.cc

maysamyabandeh · 2017-06-30T23:36:17Z

db/table_cache.cc

@@ -336,10 +337,21 @@ Status TableCache::Get(const ReadOptions& options,

    if (auto row_handle =
            ioptions_.row_cache->Lookup(row_cache_key.GetUserKey())) {
+
+      // Cleanable routine to release the cache entry
+      Cleanable cache_entry_clean;


Can you make sure to consider this comment? #2492 (comment)

Can you please apply the comment?

can you let me know what comment you are mentioning here? I have put the comment before definition

If you click on the link that is pasted in my comment:

Can you make sure to consider this comment? #2492 (comment)

it will take you to this comment:

To be consistent with the existing code, lets call it value_pinner. Lets rename it on other places too.

maysamyabandeh · 2017-06-30T23:50:28Z

db/table_cache.cc

      auto found_row_cache_entry = static_cast<const std::string*>(
          ioptions_.row_cache->Value(row_handle));
-      replayGetContextLog(*found_row_cache_entry, user_key, get_context);
-      ioptions_.row_cache->Release(row_handle);
+      // If it comes here value is located on the cache, input PinnableSlice (get_context.pinnable_slice_) will now


Lets try again to make the comment more clear.

It says "input PinnableSlice (get_context.pinnable_slice_) will now point to"? It is pointing now or it will point after replayGetContextLog?

Would be great to clarify these points in the comment:

found_row_cache_entry points to the value on cache

cache_entry_clean (that you will rename it to value_pinner) will have the cleanup procedure for the cached entry.

After replayGetContextLog, get_context.pinnable_slice_ might point to the cached entry (or a copy based on that) and the value_pinner cleanups are delegated to get_context.pinnable_slice_

The cached entry is released after get_context.pinnable_slice_ is reset.

// If it comes here value is located on the cache. // found_row_cache_entry points to the value on cache, // and cache_entry_clean has cleanup procedure for the cached entry. // After replayGetContextLog() returns get_context.pinnable_slice_ // will point to cache entry buffer ((or a copy based on that) and // cleanup routine under cache_entry_clean will be delegated to // get_context.pinnable_slice_. Cache entry is released when // get_context.pinnable_slice_ is reset

Let me know if it needs to be tweaked again

You wanted to rename cache_entry_clean to value_pinner.

Nits:
After replayGetContextLog() returns => After replayGetContextLog() returns,
((or a => (or a
is reset => is reset.

facebook-github-bot · 2017-07-06T23:41:37Z

@sushmaad updated the pull request - view changes - changes since last import

facebook-github-bot · 2017-07-10T05:52:01Z

@sushmaad updated the pull request - view changes - changes since last import

facebook-github-bot · 2017-07-12T03:21:13Z

@sushmaad updated the pull request - view changes - changes since last import

facebook-github-bot · 2017-07-12T04:21:36Z

@sushmaad updated the pull request - view changes - changes since last import

facebook-github-bot · 2017-07-12T04:37:03Z

@sushmaad updated the pull request - view changes - changes since last import

facebook-github-bot · 2017-07-12T04:42:18Z

@sushmaad updated the pull request - view changes - changes since last import

maysamyabandeh · 2017-07-12T14:37:01Z

db/table_cache.cc

-      ioptions_.row_cache->Release(row_handle);
+      // If it comes here value is located on the cache,
+      // found_row_cache_entry points to the value on cache,
+      // cache_entry_clean has cleanup procedure for the cached entry,


nit: // and cache_entry_clean has ...

To make it easier to read, lets have clear sentences with a begin and an end. In that interest, let finish this sentence here: "for the cached entry."

maysamyabandeh · 2017-07-12T14:38:13Z

db/table_cache.cc

+      // If it comes here value is located on the cache,
+      // found_row_cache_entry points to the value on cache,
+      // cache_entry_clean has cleanup procedure for the cached entry,
+      // after replayGetContextLog() returns  get_context.pinnable_slice_


after => After
"returns " => "returns " // one space is extra

maysamyabandeh · 2017-07-12T14:39:57Z

db/table_cache.cc

+      // found_row_cache_entry points to the value on cache,
+      // cache_entry_clean has cleanup procedure for the cached entry,
+      // after replayGetContextLog() returns  get_context.pinnable_slice_
+      // will point to cache entry buffer and


Can you please make this change: "entry buffer" => "entry buffer (or a copy based on that)"

maysamyabandeh · 2017-07-12T14:40:46Z

db/table_cache.cc

+      // after replayGetContextLog() returns  get_context.pinnable_slice_
+      // will point to cache entry buffer and
+      // cleanup routine under cache_entry_clean will be delegated to
+      // get_context.pinnable_slice_, cache entry is released when


", cache" => ". Cache"

maysamyabandeh · 2017-07-12T14:41:31Z

db/table_cache.cc

@@ -336,10 +337,21 @@ Status TableCache::Get(const ReadOptions& options,

    if (auto row_handle =
            ioptions_.row_cache->Lookup(row_cache_key.GetUserKey())) {
+
+      // Cleanable routine to release the cache entry
+      Cleanable cache_entry_clean;


Can you please apply the comment?

maysamyabandeh · 2017-07-12T14:42:56Z

db/table_cache.cc

+      // cleanup routine under cache_entry_clean will be delegated to
+      // get_context.pinnable_slice_, cache entry is released when
+      // get_context.pinnable_slice_ is reset
+      cache_entry_clean.RegisterCleanup(release_cache_entry_func,


Can you please apply this comment: ee5273e#r124070427

facebook-github-bot · 2017-07-13T18:38:38Z

@sushmaad updated the pull request - view changes - changes since last import

facebook-github-bot · 2017-07-14T19:01:47Z

@sushmaad updated the pull request - view changes - changes since last import

maysamyabandeh · 2017-07-14T19:19:42Z

db/db_test_util.cc

-    result = std::string(v->data());
-  }
-  return result;
+  value = std::string(v->data());


What is that you need to convert PinnableSlice to string? Seems redundant, does not it? Can't the function that calls ::Get read the value from v?

maysamyabandeh · 2017-07-14T19:28:41Z

db/db_test.cc

+    PinnableSlice pin_slice;
+    std::string value;
+    ASSERT_EQ(Get("foo", &pin_slice, value), Status::OK());
+    ASSERT_EQ(value, "bar");


Here you can do pin_slice.ToString

facebook-github-bot · 2017-07-14T20:46:32Z

@sushmaad updated the pull request - view changes - changes since last import

facebook-github-bot · 2017-07-14T20:55:01Z

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

maysamyabandeh · 2017-07-14T20:57:23Z

LGTM. Will land if the tests pass.

maysamyabandeh · 2017-07-14T22:28:32Z

@sushmaad can you run "make format" please.

facebook-github-bot · 2017-07-17T18:15:47Z

@sushmaad updated the pull request - view changes - changes since last import

facebook-github-bot · 2017-07-17T18:24:50Z

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

…ring and value

facebook-github-bot · 2017-07-17T20:22:35Z

@sushmaad updated the pull request - view changes - changes since last import

facebook-github-bot · 2017-07-17T20:26:37Z

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

facebook-github-bot · 2017-07-17T21:31:02Z

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

facebook-github-bot added the CLA Signed label Jun 24, 2017

maysamyabandeh reviewed Jun 26, 2017

View reviewed changes

maysamyabandeh requested review from IslamAbdelRahman and ot June 26, 2017 17:31

ot reviewed Jun 28, 2017

View reviewed changes

maysamyabandeh reviewed Jun 30, 2017

View reviewed changes

sushmaad changed the title ~~enable PinnacleSlice for RowCache~~ enable PinnableSlice for RowCache Jul 6, 2017

sushmaad force-pushed the enable_ps_rcache branch from 811200a to ee5273e Compare July 12, 2017 03:21

sushmaad force-pushed the enable_ps_rcache branch from a89a9d9 to ee5273e Compare July 12, 2017 04:36

maysamyabandeh reviewed Jul 12, 2017

View reviewed changes

maysamyabandeh reviewed Jul 14, 2017

View reviewed changes

maysamyabandeh approved these changes Jul 14, 2017

View reviewed changes

Sushma Devendrappa and others added 8 commits July 17, 2017 13:19

enable PinnacleSlice for RowCache

2e54098

adding unit test for using PinnableSlice with RowCache

75c51df

styling and adding more comments

08a8afd

add more comments and format

0fd71bd

fixing comments and more styling

1e15224

changing DBTestBase::Get() to return Status instead of Status code st…

42d9c5e

…ring and value

removing unwanted argument from DBTestBase::Get()

4d1bded

running make format

372568b

sushmaad force-pushed the enable_ps_rcache branch from 3092dc8 to 372568b Compare July 17, 2017 20:22

facebook-github-bot closed this in 0655b58 Jul 17, 2017

enable PinnableSlice for RowCache #2492

enable PinnableSlice for RowCache #2492

Conversation

sushmaad commented Jun 24, 2017

facebook-github-bot commented Jun 24, 2017

facebook-github-bot commented Jun 24, 2017

facebook-github-bot commented Jun 24, 2017

maysamyabandeh Jun 26, 2017 • edited Loading

Choose a reason for hiding this comment

maysamyabandeh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ot commented Jun 26, 2017

siying commented Jun 26, 2017

ot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Jun 29, 2017

facebook-github-bot commented Jun 29, 2017

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

maysamyabandeh Jul 13, 2017 • 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

facebook-github-bot commented Jul 6, 2017

facebook-github-bot commented Jul 10, 2017

facebook-github-bot commented Jul 12, 2017

facebook-github-bot commented Jul 12, 2017

facebook-github-bot commented Jul 12, 2017

facebook-github-bot commented Jul 12, 2017

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 13, 2017

facebook-github-bot commented Jul 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Jul 14, 2017

facebook-github-bot commented Jul 14, 2017

maysamyabandeh commented Jul 14, 2017

maysamyabandeh commented Jul 14, 2017

facebook-github-bot commented Jul 17, 2017

facebook-github-bot commented Jul 17, 2017

facebook-github-bot commented Jul 17, 2017

facebook-github-bot commented Jul 17, 2017

facebook-github-bot commented Jul 17, 2017

maysamyabandeh Jun 26, 2017 •

edited

Loading

maysamyabandeh Jul 13, 2017 •

edited

Loading