Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20607 )
Change subject: WIP KUDU-613: Scan Resistant Caching ...................................................................... Patch Set 4: (9 comments) It's very important to add performance tests to be able to reason about the performance of the current approach under heavy concurrent load. Please add such a test -- you could take a look at the existing test scenarios for the cache with the plain LRU eviction policy. 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: Cache::MemoryType mem_type Could this be templatized instead based on the 'mem_type'? http://gerrit.cloudera.org:8080/#/c/20607/4/src/kudu/util/slru_cache.h@46 PS4, Line 46: lookups nit: probably, 'lookup_threshold' or something similar might be a better moniker? http://gerrit.cloudera.org:8080/#/c/20607/4/src/kudu/util/slru_cache.h@51 PS4, Line 51: CHECK_GT(lookups, 0); : lookups_ = lookups; nit: initialize lookups_ in the init list above, and then run CHECK_GT() on lookups_. http://gerrit.cloudera.org:8080/#/c/20607/4/src/kudu/util/slru_cache.h@83 PS4, Line 83: simple_spinlock mutex_; Add a comment on what this synchronization primitive is used for? http://gerrit.cloudera.org:8080/#/c/20607/4/src/kudu/util/slru_cache.h@84 PS4, Line 84: uint32_t lookups_; nit: add const? 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: Cache::UniqueHandle nit: could add 'using' for this and drop the 'Cache::' prefix? http://gerrit.cloudera.org:8080/#/c/20607/4/src/kudu/util/slru_cache.cc@39 PS4, Line 39: std::lock_guard<decltype(mutex_)> l(mutex_); Is this the giant lock on all the cache or that's per-shard one? If that's a giant lock, I don't think this is going to fit the bill since this is a bottleneck for a cache that's supposed to handle very high rate of concurrent requests. I'd suggest you add performance tests to see how it fares compared with regular LRU cache -- if the concurrent performance is two times less or more than LRU cache with many shards, then this approach needs to be changed, so the locks are taken on a shard level, not the whole cache level. http://gerrit.cloudera.org:8080/#/c/20607/4/src/kudu/util/slru_cache.cc@40 PS4, Line 40: Cache::UniqueHandle protected_handle(protected_cache_->Lookup(key, caching)); : Cache::UniqueHandle probationary_handle(probationary_cache_->Lookup(key, caching)); Could this be optimized to return right away if an element find in the protected section? http://gerrit.cloudera.org:8080/#/c/20607/4/src/kudu/util/slru_cache.cc@42 PS4, Line 42: // If an entry exists, it should not exist in both caches. : CHECK(!(protected_handle && probationary_handle)); Have this only for when NDEBUG is not defined? -- 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: 4 Gerrit-Owner: Mahesh Reddy <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <[email protected]> Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 09 Nov 2023 21:03:24 +0000 Gerrit-HasComments: Yes
