Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/20607 )
Change subject: KUDU-613: Introduce SLRU cache ...................................................................... Patch Set 20: (14 comments) http://gerrit.cloudera.org:8080/#/c/20607/16//COMMIT_MSG Commit Message: PS16: > Don't forget to re-base this patch on top of the HEAD in the main branch, o Done http://gerrit.cloudera.org:8080/#/c/20607/16//COMMIT_MSG@22 PS16, Line 22: > I guess it's not a big deal to build in RELEASE configuration and run the t Done http://gerrit.cloudera.org:8080/#/c/20607/19//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20607/19//COMMIT_MSG@23 PS19, Line 23: Ran cache benchmarks for RELEASE build. : Build ran locally on macOS, 6 cores and 2.6GHz. > We need stats for RELEASE build, not DEBUG. Please add those when you have Done http://gerrit.cloudera.org:8080/#/c/20607/19/src/kudu/util/cache-bench.cc File src/kudu/util/cache-bench.cc: http://gerrit.cloudera.org:8080/#/c/20607/19/src/kudu/util/cache-bench.cc@104 PS19, Line 104: / kEntrySize; > I think this should have been (kProbationarySegmentCapacity + kProtectedSeg Good catch. I was mistaken that max_key had to do with the entry size. http://gerrit.cloudera.org:8080/#/c/20607/19/src/kudu/util/nvm_cache.cc File src/kudu/util/nvm_cache.cc: http://gerrit.cloudera.org:8080/#/c/20607/19/src/kudu/util/nvm_cache.cc@556 PS19, Line 556: UniquePendingHandle Allocate(Slice key, int val_len, int charge) override { > nit: why to change this at all? Done http://gerrit.cloudera.org:8080/#/c/20607/19/src/kudu/util/slru_cache.h File src/kudu/util/slru_cache.h: http://gerrit.cloudera.org:8080/#/c/20607/19/src/kudu/util/slru_cache.h@74 PS19, Line 74: // Removes entry from shard, frees the entry if no refs are remaining. : void Erase(const Slice& key, uint32_t hash); : // Like Erase, but underlying entry is not freed. : // Necessary when upgrading entry to protected segment. : void SoftErase(const Slice& key, uint32_t hash); : // Returns true if shard contains entry, false if not. : bool Contains(const Slice& key, uint32_t > It seems the code evolved and these methods are now used only in tests. If Done http://gerrit.cloudera.org:8080/#/c/20607/19/src/kudu/util/slru_cache.h@113 PS19, Line 113: cause the cache implementation is sharded, we do this tracking in a > The name of the method already assumes this cannot be anything but Cache::E Removed it. Only template parameter now is the MemoryType but even that will only be DRAM. What template parameters would you like to see here? http://gerrit.cloudera.org:8080/#/c/20607/19/src/kudu/util/slru_cache.cc File src/kudu/util/slru_cache.cc: http://gerrit.cloudera.org:8080/#/c/20607/19/src/kudu/util/slru_cache.cc@63 PS19, Line 63: RLHan > nit for here and elsewhere in this file: maybe, add corresponding 'using' d Done http://gerrit.cloudera.org:8080/#/c/20607/19/src/kudu/util/slru_cache.cc@66 PS19, Line 66: if (Unref(e)) { > For this and other methods that are specific to protected or probational se Added it as a TODO for now, this is a good idea, thanks for the feedback. http://gerrit.cloudera.org:8080/#/c/20607/19/src/kudu/util/slru_cache.cc@231 PS19, Line 231: return reinterpret_cast<Handle*>(handle); : } > nit: could this be just Done http://gerrit.cloudera.org:8080/#/c/20607/19/src/kudu/util/slru_cache.cc@468 PS19, Line 468: int bits = PREDICT_FALSE(FLAGS_cache_force_single_shard) ? : 0 : Bits::Log2Ceiling(base::NumCPUs()); : VLOG(1) << "Will use " << (1 << bits) << " shards for SLRU cache."; : return bits; : } : > After consideration, inserting MRU first seems counter-productive. For sim Good catch, changed it to insert the LRU entries first. Also changed this same logic in SLRUCacheShardPair::Insert(). http://gerrit.cloudera.org:8080/#/c/20607/19/src/kudu/util/slru_cache.cc@516 PS19, Line 516: : : : : > nit: it could be Done http://gerrit.cloudera.org:8080/#/c/20607/19/src/kudu/util/slru_cache.cc@526 PS19, Line 526: : > Instead of doing this, maybe just switch the order of the 'shards_' and t Done http://gerrit.cloudera.org:8080/#/c/20607/19/src/kudu/util/slru_cache.cc@539 PS19, Line 539: : : : : : : : : : : : : : : : : : : : : : : : : > This and other methods below look to be exactly identical to the code in Sh 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: 20 Gerrit-Owner: Mahesh Reddy <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <[email protected]> Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 04 Jun 2024 20:55:58 +0000 Gerrit-HasComments: Yes
