Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/20607 )
Change subject: KUDU-613: Introduce SLRU cache ...................................................................... Patch Set 14: (16 comments) http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache-test.cc File src/kudu/util/slru_cache-test.cc: http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache-test.cc@446 PS12, Line 446: while (added_weight < 0.99 * protected_segment_size_) { > Consider adding the following test scenarios as well: Added the scenarios in the first bullet point. The second scenario you mentioned is covered in Upgrade already. Let me know if the added tests are not enough. http://gerrit.cloudera.org:8080/#/c/20607/13/src/kudu/util/slru_cache-test.cc File src/kudu/util/slru_cache-test.cc: http://gerrit.cloudera.org:8080/#/c/20607/13/src/kudu/util/slru_cache-test.cc@482 PS13, Line 482: TEST_P(SLRUSingleShardCacheTest, CornerCases) { > warning: narrowing conversion from constant 'double' to 'int' [bugprone-nar Done http://gerrit.cloudera.org:8080/#/c/20607/13/src/kudu/util/slru_cache-test.cc@520 PS13, Line 520: > warning: narrowing conversion from 'unsigned long' to signed type 'int' is Done http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache.h File src/kudu/util/slru_cache.h: http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache.h@40 PS12, Line 40: NewSLRUC > It seems there are many common methods between the Cache's and SLRUCache's Made it inherit from Cache now, it's not the cleanest inheritance as there are some methods in the Cache interface there just for the SLRUCache and some methods in the Cache interface that are not used in the SLRUCache. http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache.h@89 PS12, Line 89: > nit: Maybe add the reasoning why the handle to probationary segments is ret I guess the next part of the sentence explains it, since it's the same handle being moved between segments and the protected segment lookup handle is null. But I can make it more clear. http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache.h@121 PS12, Line 121: : : : : > Why to have these two methods in the public interface? If only the impleme They're protected methods in the Cache interface now. http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache.h@134 PS12, Line 134: : : > Consider moving this into the concrete class implementation: SRLUCache is j Done http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache.cc File src/kudu/util/slru_cache.cc: http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache.cc@507 PS12, Line 507: SLRUCacheShardPair(MemTracker* mem_tracker, : size_t probationary_capacity, : size_t protected_capacity); : ~SLRUCacheShardPair(); : : void SetMetrics(CacheMetrics* probationary_metrics, CacheMetrics* protected_metrics); : > Why to have these as methods instead of supplying the capacity for the prob Done http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache.cc@526 PS12, Line 526: // mutex_ protects the state of the shard pair from other threads reading/modifying the data. : simple_spinlock mutex_; > Why to have these created on the heap, not on the stack? Please add a comm I mimicked the Cache interface's style of creating on the heap rather than the stack. I'd imagine it has to do with dynamic memory management (allocation) of entries. I'll look into this more. Changed to using unique pointers instead of raw pointers. http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache.cc@529 PS12, Line 529: > What state? Done http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache.cc@556 PS12, Line 556: urn probationary_shard_->Insert(handle, > nit: use reverse iterator instead? Done http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache.cc@558 PS12, Line 558: // If newly inserted entry has greater charge than previous one, : // possible that entries can be evicted if at capacity. : vector<Cache::RLHandle*> evictions; : auto* inserted = protected_shard_->Pr > Does it make sense to move this sanitization into the ReInsert() method? Made a separate method for this sanitization and moved the callsites into the appropriate Insert methods. http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache.cc@590 PS12, Line 590: return protected_handle; : } : > If the entry is not present in both the segments, don't we insert the entry We do insert into the probationary segment if the entry isn't present in either segment, but here since both handles are null it doesn't matter which one we return. That being said, I changed this logic in the latest patch and now it returns the probationary handle if the entry doesn't exist in either segment. http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache.cc@627 PS12, Line 627: if (ProbationaryContains(e->key(), e->hash)) { : probationary_shard_->Release(handle); : } else { : protected_shard_->Release(handle); : > Is it possible to have any race between the ProbationaryContains() check an Release() is called when handles from Lookup() go out of scope. It is possible that an entry can be looked up, and while the handle is being released it's upgraded to the protected segment by another thread. I added a lock to this method as result. Given that this method is called every time a lookup handle is out of scope, performance testing showed that adding a mutex here did result in less lookups per second (by around 50%). http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache.cc@800 PS12, Line 800: > Is it possible to store unique_ptr instead of raw pointers in the shards_ c Done http://gerrit.cloudera.org:8080/#/c/20607/13/src/kudu/util/slru_cache.cc File src/kudu/util/slru_cache.cc: http://gerrit.cloudera.org:8080/#/c/20607/13/src/kudu/util/slru_cache.cc@69 PS13, Line 69: Cache::Handle* Insert(Cache::RLHandle* handle, Cache::EvictionCallback* eviction_callback); > warning: method 'GetCapacity' can be made const [readability-make-member-fu 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: 14 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: Wed, 28 Feb 2024 09:42:12 +0000 Gerrit-HasComments: Yes
