Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20607 )
Change subject: KUDU-613: Introduce SLRU cache ...................................................................... Patch Set 19: (11 comments) First portion of comments: I'm going to post the second one later tonight. 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, otherwise it might hit logical conflicts when pushed into the git repo (i.e. risking a broken build, etc.). http://gerrit.cloudera.org:8080/#/c/20607/16//COMMIT_MSG@22 PS16, Line 22: > Pushing out this patch for now to get reviews, will work on setting up a re I guess it's not a big deal to build in RELEASE configuration and run the test, so I hope to see the updates on the stats soon. Thank you! 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 DEBUG 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 a chance. Thanks a lot! 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: nit: why to change this at all? 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: // Returns true if probationary segment contains key. : // Returns false if not. : bool ProbationaryContains(const Slice& key); : : // Returns true if protected segment contains key. : // Returns false if not. : bool ProtectedContains(const Slice& key); It seems the code evolved and these methods are now used only in tests. If so, maybe remove these from the interface and move their implementation into the test code since the tests are already marked as friends and have access to any fields and methods? Alternatively, you could add Test suffix to the names, but I guess moving these out would be cleaner. http://gerrit.cloudera.org:8080/#/c/20607/19/src/kudu/util/slru_cache.h@113 PS19, Line 113: Cache::EvictionPolicy eviction_policy = Cache::EvictionPolicy::SLRU, The name of the method already assumes this cannot be anything but Cache::EvictionPolicy::SLRU, so why to have this parameter then? 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: Cache:: nit for here and elsewhere in this file: maybe, add corresponding 'using' directives in this .cc file and omit 'Cache::' prefix? http://gerrit.cloudera.org:8080/#/c/20607/19/src/kudu/util/slru_cache.cc@231 PS19, Line 231: Cache::RLHandle* e = table_.Lookup(key, hash); : return e != nullptr; nit: could this be just return table_.Lookup(key, hash) != nullptr; http://gerrit.cloudera.org:8080/#/c/20607/19/src/kudu/util/slru_cache.cc@516 PS19, Line 516: std::unique_ptr<SLRUCacheShardPair> shard(new SLRUCacheShardPair(mem_tracker_.get(), : per_probationary_shard, : per_protected_shard, : lookups)); : shards_.push_back(std::move(shard)); nit: it could be shards_.emplace_back(new ...); http://gerrit.cloudera.org:8080/#/c/20607/19/src/kudu/util/slru_cache.cc@526 PS19, Line 526: Without it, the destructor for memtracker is called before : // the destructor for the SLRUCacheShards. Instead of doing this, maybe just switch the order of the 'shards_' and the 'mem_tracker_' fields, so the destructor of 'shards_' is called first? Also, move this text blurb in there. http://gerrit.cloudera.org:8080/#/c/20607/19/src/kudu/util/slru_cache.cc@539 PS19, Line 539: Cache::UniquePendingHandle ShardedSLRUCache::Allocate(Slice key, int val_len, int charge) { : int key_len = key.size(); : DCHECK_GE(key_len, 0); : DCHECK_GE(val_len, 0); : int key_len_padded = KUDU_ALIGN_UP(key_len, sizeof(void*)); : UniquePendingHandle h(reinterpret_cast<Cache::PendingHandle*>( : new uint8_t[sizeof(Cache::RLHandle) : + key_len_padded + val_len // the kv_data VLA data : - 1 // (the VLA has a 1-byte placeholder) : ]), : Cache::PendingHandleDeleter(this)); : Cache::RLHandle* handle = reinterpret_cast<Cache::RLHandle*>(h.get()); : handle->lookups = 0; : handle->in_protected_segment = false; : handle->key_length = key_len; : handle->val_length = val_len; : // TODO(KUDU-1091): account for the footprint of structures used by Cache's : // internal housekeeping (RL handles, etc.) in case of : // non-automatic charge. : handle->charge = (charge == Cache::kAutomaticCharge) ? kudu_malloc_usable_size(h.get()) : : charge; : handle->hash = HashSlice(key); : memcpy(handle->kv_data, key.data(), key_len); : : return h; This and other methods below look to be exactly identical to the code in ShardedCache. Could you add a TODO to mention a re-factoring opportunity to avoid the duplication of the code? There is no need to address it right away (unless you really want to do so), just add a TODO for posterity, please. Thanks! -- 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: Fri, 31 May 2024 23:48:12 +0000 Gerrit-HasComments: Yes
