From 03ccb1cd4227191d02b6794d9b0468d091a50860 Mon Sep 17 00:00:00 2001 From: Peter Dillinger Date: Tue, 4 Apr 2023 15:47:00 -0700 Subject: [PATCH] Re-clarify SecondaryCache API (#11316) Summary: I previously misread or misinterpreted API contracts for SecondaryCache and this should correct the record. (Follow-up item from https://github.com/facebook/rocksdb/issues/11301) Pull Request resolved: https://github.com/facebook/rocksdb/pull/11316 Test Plan: comments only Reviewed By: anand1976 Differential Revision: D44245107 Pulled By: pdillinger fbshipit-source-id: 3f8ddec150674b75728f1730f99b963bbf7b76e7 --- cache/secondary_cache_adapter.cc | 3 +- include/rocksdb/secondary_cache.h | 56 +++++++++++++++++-------------- 2 files changed, 31 insertions(+), 28 deletions(-) diff --git a/cache/secondary_cache_adapter.cc b/cache/secondary_cache_adapter.cc index 772b7a69a..0a96db17d 100644 --- a/cache/secondary_cache_adapter.cc +++ b/cache/secondary_cache_adapter.cc @@ -95,8 +95,7 @@ Cache::Handle* CacheWithSecondaryAdapter::Promote( PERF_COUNTER_ADD(secondary_cache_hit_count, 1); RecordTick(stats, SECONDARY_CACHE_HITS); - // FIXME? using charge=Size() is suspicious but inherited from older - // implementation + // Note: SecondaryCache::Size() is really charge (from the CreateCallback) size_t charge = secondary_handle->Size(); Handle* result = nullptr; // Insert into primary cache, possibly as a standalone+dummy entries. diff --git a/include/rocksdb/secondary_cache.h b/include/rocksdb/secondary_cache.h index b8deee66f..2aad50280 100644 --- a/include/rocksdb/secondary_cache.h +++ b/include/rocksdb/secondary_cache.h @@ -17,11 +17,23 @@ namespace ROCKSDB_NAMESPACE { -// A handle for lookup result. The handle may not be immediately ready or -// have a valid value. The caller must call isReady() to determine if its -// 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. +// A handle for lookup result. Immediately after SecondaryCache::Lookup() with +// wait=false (and depending on the implementation), the handle could be in any +// of the below states. It must not be destroyed while in the pending state. +// * Pending state (IsReady() == false): result is not ready. Value() and Size() +// must not be called. +// * Ready + not found state (IsReady() == true, Value() == nullptr): the lookup +// has completed, finding no match. Or an error occurred that prevented +// normal completion of the Lookup. +// * Ready + found state (IsReady() == false, Value() != nullptr): the lookup +// has completed, finding an entry that has been loaded into an object that is +// now owned by the caller. +// +// Wait() or SecondaryCache::WaitAll() may be skipped if IsReady() happens to +// return true, but (depending on the implementation) IsReady() might never +// return true without Wait() or SecondaryCache::WaitAll(). After the handle +// is known ready, calling Value() is required to avoid a memory leak in case +// of a cache hit. class SecondaryCacheResultHandle { public: virtual ~SecondaryCacheResultHandle() = default; @@ -36,7 +48,9 @@ class SecondaryCacheResultHandle { // the lookup was unsuccessful. virtual Cache::ObjectPtr Value() = 0; - // Return the size of value + // Return the out_charge from the helper->create_cb used to construct the + // object. + // WART: potentially confusing name virtual size_t Size() = 0; }; @@ -57,24 +71,13 @@ class SecondaryCache : public Customizable { const std::string& id, std::shared_ptr* result); - // Insert the given value into this cache. Ownership of `value` is - // transferred to the callee, who is reponsible for deleting the value - // with helper->del_cb if del_cb is not nullptr. Unlike Cache::Insert(), - // the callee is responsible for such cleanup even in case of non-OK - // Status. - // Typically, the value is not saved directly but the implementation - // uses the SaveToCallback provided by helper to extract value's - // persistable data (typically uncompressed block), which will be written - // to this tier. The implementation may or may not write it to cache - // depending on the admission control policy, even if the return status - // is success (OK). - // - // If the implementation is asynchronous or otherwise uses `value` after - // the call returns, then InsertSaved() must be overridden not to rely on - // Insert(). For example, there could be a "holding area" in memory where - // Lookup() might return the same parsed value back. But more typically, if - // the implementation only uses `value` for getting persistable data during - // the call, then the default implementation of `InsertSaved()` suffices. + // Suggest inserting an entry into this cache. The caller retains ownership + // of `obj` (also called the "value"), so is only used directly by the + // SecondaryCache during Insert(). When the cache chooses to perform the + // suggested insertion, it uses the size_cb and saveto_cb provided by + // `helper` to extract the persistable data (typically an uncompressed block) + // and writes it to this cache tier. OK may be returned even if the insertion + // is not made. virtual Status Insert(const Slice& key, Cache::ObjectPtr obj, const Cache::CacheItemHelper* helper) = 0; @@ -84,8 +87,9 @@ class SecondaryCache : public Customizable { // may or may not write it to cache depending on the admission control // policy, even if the return status is success. // - // The default implementation assumes synchronous, non-escaping Insert(), - // wherein `value` is not used after return of Insert(). See Insert(). + // The default implementation only assumes the entry helper's create_cb is + // called at Lookup() time and not Insert() time, so should work for all + // foreseeable implementations. virtual Status InsertSaved(const Slice& key, const Slice& saved); // Lookup the data for the given key in this cache. The create_cb