Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20607 )
Change subject: KUDU-613: Introduce SLRU cache ...................................................................... Patch Set 20: Code-Review+1 (4 comments) Almost there. Just a few nits. http://gerrit.cloudera.org:8080/#/c/20607/20//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20607/20//COMMIT_MSG@23 PS20, Line 23: Ran cache benchmarks for RELEASE build nit: please also note whether you were using the default flag settings or there were any custom settings http://gerrit.cloudera.org:8080/#/c/20607/20//COMMIT_MSG@25 PS20, Line 25: Here are some benchmark numbers for SLRU cache: It would be great to add a few patterns into the cache-bench.cc that clearly show the benefits of using the SLRU cache. The very obvious one is something like multiple requests to many relatively small values, sprinkled with once-at-a-time requests for relatively large values that push out many elements out of the LRU, but not from the SLRU cache. Otherwise, without presenting the actual hit ratio as an evidence, it's a hard call to sell the idea of using this SLRU implementation vs the LRU one if the former shows worse characteristics in both the lookup rate and hit ratio for all the benchmarked patterns (0.4% hit ratio gain vs LRU for ZIPFIAN ratio=1.00x looks like an aberration). Probably, adding that in a separate patch is a good option. If you have capacity to do so in this patch, that would be even better. Thanks! http://gerrit.cloudera.org:8080/#/c/20607/20/src/kudu/util/cache-bench.cc File src/kudu/util/cache-bench.cc: http://gerrit.cloudera.org:8080/#/c/20607/20/src/kudu/util/cache-bench.cc@60 PS20, Line 60: 5 nit: did you experiment with other settings for the protected segment lookup threshold? I guess the hit ratio of the SLRU vs LRU might improve a bit for the uniformly distributed keys if using 2 or 3 for the threshold setting http://gerrit.cloudera.org:8080/#/c/20607/20/src/kudu/util/slru_cache.cc File src/kudu/util/slru_cache.cc: http://gerrit.cloudera.org:8080/#/c/20607/20/src/kudu/util/slru_cache.cc@400 PS20, Line 400: ShardedSLRUCache::~ShardedSLRUCache() { }; nit: this could be replaced with the following in the header file ~ShardedSLRUCache() = default; -- 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: 20 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, 05 Jun 2024 23:48:55 +0000 Gerrit-HasComments: Yes
