Skip to content

[POC] Initial support for NVM cache in LRUCache#8113

Merged
anand1976 merged 8 commits intofacebook:nvm_cache_protofrom
anand1976:nvm_cache_initial
Apr 1, 2021
Merged

[POC] Initial support for NVM cache in LRUCache#8113
anand1976 merged 8 commits intofacebook:nvm_cache_protofrom
anand1976:nvm_cache_initial

Conversation

@anand1976
Copy link
Copy Markdown
Contributor

Defined the abstract interface for a NVM/persistent cache in include/rocksdb/nvm_cache.h, and updated LRUCacheOptions to take a std::shared_ptr<NvmCache>. An item is initially inserted into the LRU cache. When it ages out and evicted from memory, its inserted into the NVM cache. On a LRU cache miss and successful lookup in NVM, the item is promoted to the in memory cache. Only support synchronous lookup currently.

Summary:
Only support synchronous lookup currently.
Comment thread cache/lru_cache.h Outdated
Comment thread include/rocksdb/cache.h
Comment thread include/rocksdb/cache.h Outdated
Comment thread include/rocksdb/cache.h
Comment thread include/rocksdb/nvm_cache.h Outdated
Comment thread cache/lru_cache.cc

return s;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For EraseUnRefEntries() and Erase, do we need to also consider adding the entry to NVM cache?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or if we call Erase(), we remove the entry from both block cache and NVM cache?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Erase is mostly called for entries that are no longer valid (when a file is deleted for example), so we need to also erase from the NVM cache. I'll defer the implementation to a follow-on PR.

Copy link
Copy Markdown
Contributor Author

@anand1976 anand1976 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review @zhichao-cao

Comment thread cache/lru_cache.cc

return s;
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Erase is mostly called for entries that are no longer valid (when a file is deleted for example), so we need to also erase from the NVM cache. I'll defer the implementation to a follow-on PR.

Comment thread include/rocksdb/cache.h
Comment thread include/rocksdb/nvm_cache.h Outdated
@zhichao-cao
Copy link
Copy Markdown
Contributor

Thanks for addressing the previous comments. Also, the PR description may need to be modified to reflect the tiered cache designs.

// ready, and call Wait() in order to block until it becomes ready.
// The caller must call value() after it becomes ready to determine if the
// handle successfullly read the item.
class TieredCacheHandle {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So in the regular case, use need to have a loop to check isReady() and if not, sleep the thread?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not necessarily a loop, but yes, check isReady() and then call Wait().

Comment thread include/rocksdb/tiered_cache.h Outdated

virtual std::string Name() = 0;

// Insert the given value into the NVM cache. The value is not written
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May need to update the comments accordingly into Tiered Cache (instead of NVM cache) at multiple places.

Comment thread cache/lru_cache.cc
Comment thread cache/lru_cache.cc
@zhichao-cao
Copy link
Copy Markdown
Contributor

Thanks for addressing the comments, the PR looks good to me.

Comment thread cache/lru_cache.cc
}

// If handle table lookup failed, then allocate a handle outside the
// mutex if we're going to lookup in the NVM cache
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NVM cache-> tiered cache

Comment thread include/rocksdb/cache.h
enum class Priority { HIGH, LOW };

// A set of callbacks to allow objects in the volatile block cache to be
// be persisted in a NVM cache tier. Since the volatile cache holds C++
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NVM cache tier -> tiered cache (e.g., NVM cache tier)

@anand1976 anand1976 requested a review from riversand963 April 1, 2021 17:30
Comment thread include/rocksdb/cache.h
// object itself.
//
// The SizeCallback takes a void* pointer to the object and returns the size
// of the persistable data. It can be used by the NVM cache to allocate
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also replacing the "NVM cache" -> "tiered ache" in multiple places here

@anand1976 anand1976 removed the request for review from riversand963 April 1, 2021 17:30
@anand1976
Copy link
Copy Markdown
Contributor Author

Cc @riversand963

Comment thread include/rocksdb/cache.h
// function.
virtual Handle* Lookup(const Slice& key, Statistics* stats = nullptr) = 0;

// Lookup the key in the volatile and NVM tiers (if one is configured).
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NVM tiers -> tiered cache layers

Copy link
Copy Markdown
Contributor

@zhichao-cao zhichao-cao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGMT, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants