Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/20607 )
Change subject: KUDU-613: Scan Resistant Caching ...................................................................... Patch Set 5: (9 comments) http://gerrit.cloudera.org:8080/#/c/20607/4/src/kudu/util/slru_cache.h File src/kudu/util/slru_cache.h: http://gerrit.cloudera.org:8080/#/c/20607/4/src/kudu/util/slru_cache.h@46 PS4, Line 46: rded LRU caches with a fix > Could this be templatized instead based on the 'mem_type'? I need access to the mem_type for some logic in slru_cache. DRAM-based cache uses a RLHandle while the NVM-based cache uses an LRUHandle. http://gerrit.cloudera.org:8080/#/c/20607/4/src/kudu/util/slru_cache.h@46 PS4, Line 46: onary_c > nit: probably, 'lookup_threshold' or something similar might be a better mo Done http://gerrit.cloudera.org:8080/#/c/20607/4/src/kudu/util/slru_cache.h@51 PS4, Line 51: SLRUCache(size_t probationary_capacity, size_t protected_capacity, : Cache::Memo > nit: initialize lookups_ in the init list above, and then run CHECK_GT() on Done http://gerrit.cloudera.org:8080/#/c/20607/4/src/kudu/util/slru_cache.h@83 PS4, Line 83: // entry is erased from > Add a comment on what this synchronization primitive is used for? Done http://gerrit.cloudera.org:8080/#/c/20607/4/src/kudu/util/slru_cache.h@84 PS4, Line 84: // inserted into t > nit: add const? Done http://gerrit.cloudera.org:8080/#/c/20607/4/src/kudu/util/slru_cache.cc File src/kudu/util/slru_cache.cc: http://gerrit.cloudera.org:8080/#/c/20607/4/src/kudu/util/slru_cache.cc@25 PS4, Line 25: namespace kudu { > nit: could add 'using' for this and drop the 'Cache::' prefix? Tried, but can't use 'using' for class members. http://gerrit.cloudera.org:8080/#/c/20607/4/src/kudu/util/slru_cache.cc@39 PS4, Line 39: > Is this the giant lock on all the cache or that's per-shard one? If that's It's a giant lock for now, I'll look into performance testing with this approach this weekend. How would locking on the per shard level work for this use case? On a shard level within both the probationary and the protected cache, locks already exist, but this giant lock just prevents any actions from taking place on the entire slru cache during the lookup operation since entries can be erased, allocated/inserted, and lookups exist. Maybe I don't quite fully understand how the per shard level locking would look here. http://gerrit.cloudera.org:8080/#/c/20607/4/src/kudu/util/slru_cache.cc@40 PS4, Line 40: std::lock_guard<decltype(mutex_)> l(mutex_); : Cache::UniqueHandle probationary_handle(probationary_cache_->Lookup(key, caching)); > Could this be optimized to return right away if an element find in the prot It could but then we can't check the condition below, checking that the entry doesn't exist in both the probationary and protected cache. http://gerrit.cloudera.org:8080/#/c/20607/4/src/kudu/util/slru_cache.cc@42 PS4, Line 42: Cache::UniqueHandle protected_handle(protected_cache_->Lookup(key, caching)); : // If an entry exists, it should not exist in both > Have this only for when NDEBUG is not defined? 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: 5 Gerrit-Owner: Mahesh Reddy <mre...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <ale...@apache.org> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <mre...@cloudera.com> Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Fri, 10 Nov 2023 00:33:07 +0000 Gerrit-HasComments: Yes
