Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/20607 )
Change subject: KUDU-613: Introduce SLRU cache ...................................................................... Patch Set 19: (12 comments) http://gerrit.cloudera.org:8080/#/c/20607/16//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20607/16//COMMIT_MSG@22 PS16, Line 22: > Thanks for clarifying. Pushing out this patch for now to get reviews, will work on setting up a release build to get accurate stats. http://gerrit.cloudera.org:8080/#/c/20607/14/src/kudu/util/cache.h File src/kudu/util/cache.h: http://gerrit.cloudera.org:8080/#/c/20607/14/src/kudu/util/cache.h@228 PS14, Line 228: // original metrics. : kReset, : }; : // Set the cache metrics to update corresponding counters accordingly. : virtual void SetMetrics(std::unique_ptr<CacheMetrics> metrics, : ExistingMetricsPolicy metrics_policy) = 0; : : // Opaque handle to an entry stored in the cache. : struct Handle { }; : : // Custom deleter: intended for use with std::unique_ptr<Handle>. : class HandleDeleter { : public: : explicit HandleDeleter(Cache* c) : : c_(c) { : } : : void operator()(Handle* h) const { : if (h != nullptr) { > OK, I guess one can try to circle back on the attempts to clean up this mes Changed the SLRUCache metrics to inherit from CacheMetrics, so this method is no longer necessary., http://gerrit.cloudera.org:8080/#/c/20607/18/src/kudu/util/slru_cache-test.cc File src/kudu/util/slru_cache-test.cc: http://gerrit.cloudera.org:8080/#/c/20607/18/src/kudu/util/slru_cache-test.cc@280 PS18, Line 280: try t > nit: Although self explanatory, it'd good to have a comment to brief what i Done http://gerrit.cloudera.org:8080/#/c/20607/18/src/kudu/util/slru_cache-test.cc@442 PS18, Line 442: added_weight += weight; > It would be great to add a comment to explain what 'downgrade eviction' mea Done http://gerrit.cloudera.org:8080/#/c/20607/16/src/kudu/util/slru_cache.h File src/kudu/util/slru_cache.h: http://gerrit.cloudera.org:8080/#/c/20607/16/src/kudu/util/slru_cache.h@40 PS16, Line 40: emTracker; > Those are the usual benefits of having common approach: this is to have mor Done http://gerrit.cloudera.org:8080/#/c/20607/16/src/kudu/util/slru_cache.cc File src/kudu/util/slru_cache.cc: http://gerrit.cloudera.org:8080/#/c/20607/16/src/kudu/util/slru_cache.cc@57 PS16, Line 57: > IIUC, you could get away from pollution by using std::enable_if construct, I ended up moving all the SLRUCache specific methods outside of the Cache interface, so at least that looks cleaner now. http://gerrit.cloudera.org:8080/#/c/20607/17/src/kudu/util/slru_cache.cc File src/kudu/util/slru_cache.cc: http://gerrit.cloudera.org:8080/#/c/20607/17/src/kudu/util/slru_cache.cc@581 PS17, Line 581: > Whoops, the comment is lost, indeed. Done http://gerrit.cloudera.org:8080/#/c/20607/17/src/kudu/util/slru_cache.cc@601 PS17, Line 601: : bool ShardedSLRUCache::ProbationaryContains(const Slice& key) { : const uint32_t hash = HashSlice(key); : return shards_[Shard(hash)]->ProbationaryContains(key, hash); : } : : bool > I'm not sure I understand: if each of the elements is of std::unique_ptr<> The issue doesn't manifest itself in the unit tests, but it does when running the cache benchmarks. The SLRUCacheShard destructor updates the mem_tracker to release all the memory consumed. But without this custom destructor seen in ShardedSLRUCache, the destructor for mem_tracker is called before the SLRUCacheShard desutrctor. Then in the mem_tracker destructor the check for unreleased consumption == 0 fails. http://gerrit.cloudera.org:8080/#/c/20607/18/src/kudu/util/slru_cache.cc File src/kudu/util/slru_cache.cc: http://gerrit.cloudera.org:8080/#/c/20607/18/src/kudu/util/slru_cache.cc@188 PS18, Line 188: if (new_deferred > max_deferred_consumption_ || : new_deferred < -max_deferred_consumption_) { : int64_t to_propagate = deferred_consumption_.exchange(0, std::memory_order_relaxed); : mem_tracker_->Consume(to_propagate); : } > Is it possible to avoid code repetition here and SoftFreeEntry()? Removed metrics from this patch, will be added in follow up patch. But will work in your feedback there. http://gerrit.cloudera.org:8080/#/c/20607/18/src/kudu/util/slru_cache.cc@448 PS18, Line 448: > Could this be 'const'? Done http://gerrit.cloudera.org:8080/#/c/20607/18/src/kudu/util/slru_cache.cc@460 PS18, Line 460: return probationary_handle; : } : : // Upgrade from the probationary segment. : // Erase entry from the probationary segment then add entry to the > nit: fix the indent Done http://gerrit.cloudera.org:8080/#/c/20607/18/src/kudu/util/slru_cache.cc@574 PS18, Line 574: > style nit: fix the indent Done -- To view, visit http://gerrit.cloudera.org:8080/20607 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I45531534a2049dd38c002f4dc7982df9fd46e0bb Gerrit-Change-Number: 20607 Gerrit-PatchSet: 19 Gerrit-Owner: Mahesh Reddy <mre...@cloudera.com> Gerrit-Reviewer: Abhishek Chennaka <achenn...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <ale...@apache.org> Gerrit-Reviewer: Attila Bukor <abu...@apache.org> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <mre...@cloudera.com> Gerrit-Reviewer: Marton Greber <greber...@gmail.com> Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 30 May 2024 17:08:57 +0000 Gerrit-HasComments: Yes
