Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12825 )
Change subject: WIP [util] introduce TTL cache ...................................................................... Patch Set 3: (27 comments) http://gerrit.cloudera.org:8080/#/c/12825/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12825/3//COMMIT_MSG@12 PS3, Line 12: Expired entries are not returned from the cache, but kept around until : they are lazily purged upon cache reaching its capacity while : accommodating more entries. Curious if you want to introduce a singleton thread that wakes up periodically to purge expired entries. Otherwise the cache capacity is never reclaimed, even if the only workload to use it is long gone. http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache-test.cc File src/kudu/util/ttl_cache-test.cc: http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache-test.cc@55 PS3, Line 55: using TTLTestCache = TTLCache<K, TestValue>; Seems like you could hardcode string as the key too, given how you're testing. http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache-test.cc@62 PS3, Line 62: FLAGS_cache_force_single_shard = true; : FLAGS_cache_memtracker_approximation_ratio = 0; Doc why you set these. http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache-test.cc@68 PS3, Line 68: auto h = cache.Get("key0"); : ASSERT_EQ(nullptr, h.get()); Could combine negative lookups onto one line. Below too. http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache-test.cc@118 PS3, Line 118: ASSERT_NE(nullptr, put_h.get()); : ASSERT_NE(nullptr, put_h->ptr()); : ASSERT_EQ(100, put_h->ptr()->value); This seems to be a common pattern; maybe encapsulate in a helper method? http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache-test.cc@155 PS3, Line 155: auto h = cache.Get(key); : ASSERT_NE(nullptr, h.get()); : ASSERT_NE(nullptr, h->ptr()); : ASSERT_EQ(i, h->ptr()->value); Any concern here (or on L170) that the test will run sufficiently slow that the entries may be evicted by the time we call Get()? Could you loop in TSAN mode with stress threads? http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache-test.cc@201 PS3, Line 201: ASSERT_NE(nullptr, h.get()); Unnecessarily duplicated http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h File src/kudu/util/ttl_cache.h: http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@40 PS3, Line 40: // A TTL cache. It's a cache of limited capacity with FIFO eviction policy. : // All cache's entries have the same TTL. The TTL for cache's entries is : // specified at the creation time of the cache and does not change during its : // lifetime. Maybe reword as: "Cache with TTL-based time eviction and FIFO-based capacity eviction. All entries have the same static TTL specified at cache creation time." Should also mention here (or maybe in Get()/Put()) how returned handles affect eviction (either time or capacity based) as well as destruction of the underlying cached value. I think the answers are "cached key/value pairs may still be evicted even with an outstanding handle, but a cached value won't be destroyed until a returned value goes out of scope". http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@59 PS3, Line 59: V* ptr() const { : return ptr_; : } Why offer this at all? Is mutating a cached key's value an important use case? http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@91 PS3, Line 91: // TODO(aserbin): maybe, prohibit inheriting from this class instead? I don't think we need to be super protective on these matters; I think it's safe to assume that nobody is going to extend this class, so a virtual destructor is unnecessary. http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@93 PS3, Line 93: : // Retrieve an entry from the cache and return it wrapped into a ref-counting : // handle. If a non-expired entry exists for the specified key, this method : // returns shared pointer for non-null handle. If there is no entry for the : // specified key or the entry has expired at the time of the query, : // nullptr wrapped into a ref-counted handle is returned. This (and Put()) is a little too much detail. The general "retrieves...if an entry exists...if no entry exists" structure is good, but I think you can elide some of the associated detail, especially since the implementations are inline and can be inspected easily. http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@99 PS3, Line 99: std::shared_ptr<EntryHandle> Get(const K& key) { Why do Get()/Put() wrap EntryHandle in shared_ptr? The TTLCache itself doesn't actually enable sharing of EntryHandles; it looks like it always returns a newly allocated one. http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@114 PS3, Line 114: // TODO(aserbin) use make_shared? Yes, we should do this to reduce the number of allocations. http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@115 PS3, Line 115: return std::shared_ptr<EntryHandle>( Something here seems off. If I Get() the same key twice, and let both EntryHandles go out of scope, won't I end up double freeing the cached value? Can you add a test for this? http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@123 PS3, Line 123: The 'charge' parameter specifies : // the charge to associate with the entry with regard to the overall : // cache's capacity. What's the use case for this vs. letting the charge get calculated automatically based on memory consumption? http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@126 PS3, Line 126: std::shared_ptr<EntryHandle> Put(const K& key, Curious why Put() returns an EntryHandle instead of void? What's the use case you're building for (and maybe doc it)? http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@129 PS3, Line 129: // TODO(aserbin): add footprint for structures used by Cache's : // internal housekeeping (RL handles, etc.) This is KUDU-1091, right? Does it need to be tracked in TTLCache as well as in Cache? http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@131 PS3, Line 131: Cache::PendingHandle* pending = cache_->Allocate(key, sizeof(Entry), charge); Could you store this in a unique_ptr that uses Cache::Free as a deleter? Then move it into the Insert() call on L139? http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@132 PS3, Line 132: if (!pending) { : LOG(WARNING) << strings::Substitute( : "failed to allocate an entry with charge of $0", charge); : return nullptr; : } Maybe just CHECK on this instead? http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@138 PS3, Line 138: memcpy(cache_->MutableValue(pending), &entry, sizeof(Entry)); Right now, we: 1. Allocate space for an Entry in Allocate(). 2. Declare another Entry on the stack and populate it. 3. Copy the stack-allocated Entry into the cache-allocated Entry. Instead, could we cast the result of MutableValue() into an Entry*, then populate it directly? Should let us avoid the copy in step 3. http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@139 PS3, Line 139: Cache::UniqueHandle h(cache_->Insert(pending, eviction_cb_.get()), This also evicts an existing entry with key 'key', right? Maybe doc that? http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@151 PS3, Line 151: // Pointer to the 'value'. Should probably say something about ownership here. Would it even be possible to convert val_ptr into a unique_ptr? Or does that not work? http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@157 PS3, Line 157: // The eviction callback that the underlying FIFO cache uses to notify : // about reaching zero reference count for its entries. Reword as "Callback invoked by the underlying FIFO cache when a cached entry's reference count reaches zero and is evicted." http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@166 PS3, Line 166: auto* entry_ptr = reinterpret_cast<Entry*>(val.mutable_data()); : DCHECK(entry_ptr); : delete entry_ptr->val_ptr; Nit: rather than the explicit delete, maybe cast and store into a unique_ptr, then just let it go out of scope? http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache_metrics.cc File src/kudu/util/ttl_cache_metrics.cc: PS3: Seems inappropriate for these metrics to be defined for the "TTL cache", because that's generic enough that you could imagine it being used for a number of different purposes. Like, wouldn't these metrics be better suited for the Sentry privilege cache? Then if someone used TTLCache for yet another cache, we could differentiate between the two. You could make a similar argument for FileCache but I think it's weaker, because the point of the FileCache is to limit the number of open fds, and to do that effectively you'd need one instance for all file open/close. http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache_metrics.cc@41 PS3, Line 41: cache_misses ttl_cache_misses http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache_metrics.cc@49 PS3, Line 49: cache_hits ttl_cache_hits -- To view, visit http://gerrit.cloudera.org:8080/12825 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic8aa0ebe6b26bf34ca0e02bebbdb6a94f6b00621 Gerrit-Change-Number: 12825 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Mon, 25 Mar 2019 22:56:59 +0000 Gerrit-HasComments: Yes
