Add erase option to release cache #2180

maysamyabandeh · 2017-04-19T19:37:25Z

This is useful when we put the entries in the block cache for accounting
purposes and do not expect it to be used after it is released. If the cache does not
erase the item in such cases not only the performance of cache is
negatively affected but the item's destructor not being called at the
time of release might violate the assumptions about the lifetime of the
object.

The new change adds a force_erase option to the Release method and
returns a boolean to indicate whehter the item is successfully deleted.

This is useful when we put the entries in the block cache for accounting purposes and do not expect it to be used later. If the cache does not erase the item in such cases not only the performance of cache is negatively affected but the item's destructor not being called at the time of release might violate the assumptions about the lifetime of the object. The new change adds a force_erase option to the Release method and returns a boolean to indicate whehter the item is successfully deleted.

maysamyabandeh · 2017-04-19T19:39:01Z

cache/clock_cache.cc

+  if (!force_erase || erased) {
+    return erased;
+  } else {
+    return EraseAndConfirm(handle->key, handle->hash);


@yiwu-arbug I think this implementation is correct. Feel free to suggest a more optimized version if you see it applies. Thanks

Pass context to EraseAndConfirm(), and call Cleanup(context) after that.

What would be the purpose of this change? Would it improve performance?

Just a bit cleaner.

facebook-github-bot · 2017-04-19T19:40:35Z

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

sagar0

I am curious, under what cases might one want to evict data immediately, instead of doing a lazy eviction.

yiwu-arbug

After a second thought I'm not sure if the force_erase flag will solve your problem. Say you erase the filter from cache before destructing a table, while something else holds a handle of the filter, the filter can still be destruct after the table has been destructed. Can you explain more how to get around it?

yiwu-arbug · 2017-04-20T01:32:19Z

cache/clock_cache.cc

  CleanupContext context;
  CacheHandle* handle = reinterpret_cast<CacheHandle*>(h);
  Unref(handle, true, &context);
  Cleanup(context);
+  auto erased = context.to_delete_value.size() > 0;


Update Unref() to return if item erased?

yiwu-arbug · 2017-04-20T01:32:54Z

cache/clock_cache.cc

+  if (!force_erase || erased) {
+    return erased;
+  } else {
+    return EraseAndConfirm(handle->key, handle->hash);


Pass context to EraseAndConfirm(), and call Cleanup(context) after that.

yiwu-arbug · 2017-04-20T01:36:33Z

cache/lru_cache.cc

        // the cache is full
        // The LRU list must be empty since the cache is full
-        assert(lru_.next == &lru_);
+        assert(!(usage_ > capacity_) || lru_.next == &lru_);


assert(force_erase || lru_.next == &lru_) ?

That would be less strict that the current assert because even when force_erase it might be the case that usage_ > capacity_ and we do not want to skip lru_.next == &lru_ for that.

maysamyabandeh · 2017-04-20T02:31:04Z

@yiwu-arbug Why would anybody else have a handle to the object that i) I have created, and ii) I am the only consumer? In the case of filters, if anybody else try to read the same filter, that is a bug. The return value of ::Release is meant to be used for this purpose, the releaser would assert that the object is not erroneously referenced by others.

maysamyabandeh · 2017-04-20T02:39:03Z

@sagar0 The two use cases that I mentioned in the PR description are applicable. An example would be the partitioned filter of table that should be deleted before the table is closed.

yiwu-arbug · 2017-04-20T04:59:39Z

@maysamyabandeh I see. I forget that the table is the only accessor of the filter.

yiwu-arbug · 2017-04-20T05:04:52Z

cache/clock_cache.cc

@@ -637,6 +649,7 @@ void ClockCacheShard::Erase(const Slice& key, uint32_t hash) {
    }
  }
  Cleanup(context);
+  return context.to_delete_value.size() > 0;


Update UnsetInCache() to return if item erased?

facebook-github-bot · 2017-04-20T15:55:02Z

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

facebook-github-bot · 2017-04-21T02:13:49Z

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

yiwu-arbug

LGTM. I forgot to accept it.

maysamyabandeh · 2017-04-24T18:11:08Z

The failures in travis seems irrelevant:

db/db_compaction_test.cc:1269: Failure
Expected: (SizeAtLevel(i)) <= (target_size), actual: 43239165 vs 41943040
Run: org.rocksdb.WriteBatchThreadedTest testing now -> threadedWrites[WriteBatchThreadedTest(threadCount=50)] 
Assertion failed: (content_flags_.load(std::memory_order_relaxed) & (ContentFlags::DEFERRED | ContentFlags::HAS_PUT)), function Iterate, file db/write_batch.cc, line 385.

Moving on with landing...

maysamyabandeh requested review from yiwu-arbug and siying April 19, 2017 19:37

facebook-github-bot added the CLA Signed label Apr 19, 2017

maysamyabandeh commented Apr 19, 2017

View reviewed changes

sagar0 reviewed Apr 19, 2017

View reviewed changes

yiwu-arbug reviewed Apr 20, 2017

View reviewed changes

apply comments

4d448e3

yiwu-arbug approved these changes Apr 24, 2017

View reviewed changes

facebook-github-bot closed this in 4c9447d Apr 24, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add erase option to release cache #2180

Add erase option to release cache #2180

maysamyabandeh commented Apr 19, 2017 •

edited

Loading

maysamyabandeh Apr 19, 2017

yiwu-arbug Apr 20, 2017 •

edited

Loading

maysamyabandeh Apr 20, 2017

yiwu-arbug Apr 20, 2017

facebook-github-bot commented Apr 19, 2017

sagar0 left a comment

yiwu-arbug left a comment

yiwu-arbug Apr 20, 2017

yiwu-arbug Apr 20, 2017 •

edited

Loading

yiwu-arbug Apr 20, 2017

maysamyabandeh Apr 20, 2017

yiwu-arbug Apr 20, 2017

maysamyabandeh commented Apr 20, 2017

maysamyabandeh commented Apr 20, 2017

yiwu-arbug commented Apr 20, 2017

yiwu-arbug Apr 20, 2017

facebook-github-bot commented Apr 20, 2017

facebook-github-bot commented Apr 21, 2017

yiwu-arbug left a comment

maysamyabandeh commented Apr 24, 2017

Add erase option to release cache #2180

Add erase option to release cache #2180

Conversation

maysamyabandeh commented Apr 19, 2017 • edited Loading

Choose a reason for hiding this comment

yiwu-arbug Apr 20, 2017 • edited Loading

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 Apr 19, 2017

sagar0 left a comment

Choose a reason for hiding this comment

yiwu-arbug left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yiwu-arbug Apr 20, 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

maysamyabandeh commented Apr 20, 2017

maysamyabandeh commented Apr 20, 2017

yiwu-arbug commented Apr 20, 2017

Choose a reason for hiding this comment

facebook-github-bot commented Apr 20, 2017

facebook-github-bot commented Apr 21, 2017

yiwu-arbug left a comment

Choose a reason for hiding this comment

maysamyabandeh commented Apr 24, 2017

maysamyabandeh commented Apr 19, 2017 •

edited

Loading

yiwu-arbug Apr 20, 2017 •

edited

Loading

yiwu-arbug Apr 20, 2017 •

edited

Loading