Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20607 )
Change subject: KUDU-613: Introduce SLRU cache ...................................................................... Patch Set 14: (10 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: > This was a DEBUG build run locally on macOS. 6 cores and 2.6 GHz. I'll try Thanks for clarifying. We are mostly interested in RELEASE build stats in the context of performance evaluation. DEBUG and sanitizer builds don't provide any meaningful stats since nobody runs DEBUG builds in production :) You can always create create a VM in hosted or public cloud providers, build Kudu from scratch there and run performance tests. I think it's essential to have the actual stats for the lookup operations provides for a RELEASE build in this description. 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: // Sets probationary and protected segments metrics to update corresponding counters accordingly. : // : // An upgrade of one entry will increment the number of evictions from the probationary segment by : // one and the number of insertions into the protected segment by one. : // : // A downgrade of one entry will increment the number of evictions from the protected segment by : // one and the number of insertions into the probationary segment by one. : // : // To calculate the number of inserts for the SLRU cache, subtract the number of evictions : // of the protected segment from the number of inserts of the probationary segment. : // : // To calculate the number of evictions for the SLRU cache, subtract the number of insertions : // of the protected segment from the number of evictions of the probationary segment. : // : // Other than insertions and evictions, to calculate the metric for the SLRU cache, add the : // corresponding metric from the probationary and protected segment together. : virtual void SetSegmentMetrics(std::unique_ptr<CacheMetrics> probationary_metrics, : std::unique_ptr<CacheMetrics> protected_metrics, : ExistingMetricsPolicy metrics_policy) = 0; > SetMetrics() is used in other areas of the code that use a cache (file_cach OK, I guess one can try to circle back on the attempts to clean up this mess and bad smell in a follow-up patch. In this iteration, please add a TODO for the extra methods that are specific to the SLRU cache. One approach might be adding extra interface(s) and moving the specific methods out of the common Cache interface. Thank you! 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@442 PS18, Line 442: const int weight = static_cast<int>(protected_segment_size_ / 100); It would be great to add a comment to explain what 'downgrade eviction' means along with short description on what essential invariants we have to track in this scenario and why. 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: NewSLRUCache > I guess then the question is what are the template parameters? It makes sen Those are the usual benefits of having common approach: this is to have more uniform code when instantiating cache, and it automatically helps in a templatized code. It could also help with differences in the signatures of the constructors. For example, one extra level of indirection to help with the different signatures of the constructors might be introducing CacheParameters template and hiding differences in the set of parameters for the constructors. That's a common way of doing so -- you could check SslTypeTraits in openssl_util.h for an inspiration. I guess we could keep this as-is for now, especially if you are not fond of getting rid of the code ugliness in this patch itself. There might be an opportunity to clean this up later on, but most likely this will stay as ugly as its left in the first version :) 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: class SLRUCacheShard { > It's definitely possible, however, the cache file would be even more pollut IIUC, you could get away from pollution by using std::enable_if construct, so only classes of particular template parameter get particular methods instantiated. That's orthogonal to the idea of inheritance and will require updates in several other places, though. I agree that a detailed plan is needed to switch to a particular way of consolidating all this feedback on improving the structure of the cache code. My concern is that the cache code becomes more and more hairy, but I had high hopes to keep it cleaner. The first step into the spaghetti realm was with the introduction of the NVRAM-backed cache, as you could see in your attempt to unify on common data structures in your preliminary patches. Once somebody try adding SLRU NVRAM-backed cache later one, it will diverge even further. For some reason I naively thought this work might be a good opportunity to clean up the cache code. Some cleanup has been done, but it seems the rest of it is better to bring in in separate follow-up steps. To sum this up: I'm OK with keeping it as-is, especially if there is a pressure to bring in the functionality sooner than later: it's been 18 revisions already. 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: > I'm not sure I understand the proposed suggestion here. A clarification wou Whoops, the comment is lost, indeed. I guess that nit was about keeping each of the initializer's values in its down line. http://gerrit.cloudera.org:8080/#/c/20607/17/src/kudu/util/slru_cache.cc@601 PS17, Line 601: // in probationary segment to avoid evicting any entries in the protected segment. : if (val_handle->lookups < lookups_threshold || : val_handle->charge > protected_shard_->GetCapacity()) { : return probationary_handle; : } : : / > The mem tracker doesn't get properly updated unless we explicitly clear eac I'm not sure I understand: if each of the elements is of std::unique_ptr<> type, the calling reset(nullptr) on it effectively invokes the destructor of the SLRUCacheShardPair class, and nothing else, right? But that's exactly what happens upon destroying ShardedSLRUCache::shards_ when its destructor is being invoked, no? The only difference is that the order of calling the destructors are reverse compared with the extra code there. Anyway, if there is any non-trivial logic behind, please add a comment about that. 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@448 PS18, Line 448: Could this be 'const'? http://gerrit.cloudera.org:8080/#/c/20607/18/src/kudu/util/slru_cache.cc@460 PS18, Line 460: // We free the entries here outside mutex for performance reasons. : while (to_remove_head != nullptr) { : Cache::RLHandle* next = to_remove_head->next; : FreeEntry(to_remove_head); : to_remove_head = next; nit: fix the indent http://gerrit.cloudera.org:8080/#/c/20607/18/src/kudu/util/slru_cache.cc@574 PS18, Line 574: style nit: fix the indent -- 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: 14 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: Wed, 22 May 2024 02:25:41 +0000 Gerrit-HasComments: Yes
