Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/21601 )
Change subject: KUDU-613: SLRU Cache Benchmark ...................................................................... Patch Set 1: (7 comments) http://gerrit.cloudera.org:8080/#/c/21601/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21601/1//COMMIT_MSG@10 PS1, Line 10: the SLRU cache nit: the performance of the SLRU cache ? http://gerrit.cloudera.org:8080/#/c/21601/1//COMMIT_MSG@18 PS1, Line 18: Ran benchmarks for RELEASE build locally on macOS. : 6 cores and 2.6GHz. Since this patch changes the ratio for the SLRU cache segments, it would be great to add the stats on the UNIFORM and ZIPFIAN patterns as well at least to see how they fare compared with the new pattern. http://gerrit.cloudera.org:8080/#/c/21601/1/src/kudu/util/cache-bench.cc File src/kudu/util/cache-bench.cc: http://gerrit.cloudera.org:8080/#/c/21601/1/src/kudu/util/cache-bench.cc@59 PS1, Line 59: static constexpr int kProtectedSegmentCapacity = 820 * 1024 * 1024; nit: would it be easier to read like below? static constexpr int kProtectedSegmentCapacity = kCacheCapacity - kProbationarySegmentCapacity; http://gerrit.cloudera.org:8080/#/c/21601/1/src/kudu/util/cache-bench.cc@73 PS1, Line 73: // Items are pre-determined. : PRE_DETERMINED This is some particular access pattern, right? Maybe, find a proper word to describe it in addition to the fact it's pre-determined? http://gerrit.cloudera.org:8080/#/c/21601/1/src/kudu/util/cache-bench.cc@136 PS1, Line 136: if (setup.pattern == BenchSetup::Pattern::ZIPFIAN) { : int_key = r.Skewed(Bits::Log2Floor(setup.max_key())); : } else if (setup.pattern == BenchSetup::Pattern::UNIFORM) { : int_key = r.Uniform(setup.max_key()); : } else { : if (often) { : auto small_multiplier = r.Uniform(256); : int_key = large_number * small_multiplier; : } else { : // Rare random key with big value. : int_key = r.Uniform(setup.max_key()); : } : } Maybe, it's time to rewrite this piece using the 'switch' statement? http://gerrit.cloudera.org:8080/#/c/21601/1/src/kudu/util/cache-bench.cc@215 PS1, Line 215: 256 Create a constant for this and use it here and at line 142 as well? http://gerrit.cloudera.org:8080/#/c/21601/1/src/kudu/util/cache-bench.cc@216 PS1, Line 216: large_number Maybe, move this and large_number_max into BenchSetup, avoid passing on extra parameters to RunQueryThreads()? -- To view, visit http://gerrit.cloudera.org:8080/21601 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1c128a9f047497373ce3e740056eaa89a352261b Gerrit-Change-Number: 21601 Gerrit-PatchSet: 1 Gerrit-Owner: Mahesh Reddy <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 19 Jul 2024 01:07:01 +0000 Gerrit-HasComments: Yes
