Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/21601 )
Change subject: KUDU-613: SLRU Cache Benchmark ...................................................................... Patch Set 2: (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: erformance of > nit: the performance of the SLRU cache ? Done 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 Added those stats as well. Note, the SLRU cache doesn't perform well at all for the UNIFORM pattern with a cache whose protected segment is 80% of its capacity. That's to be expected as most items won't be upgraded with a UNIFORM pattern so the cache is essentially 5 times smaller only using the probationary segment which is 20% of its capacity. I remember a while ago you mentioned making the protected segment dynamically grow, that would certainly help in a case like this. That being said, it performs a lot better with a smaller value for lookups as the entire cache would be utilized earlier. 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 = kCacheCapacity - k > nit: would it be easier to read like below? Done http://gerrit.cloudera.org:8080/#/c/21601/1/src/kudu/util/cache-bench.cc@73 PS1, Line 73: UNIFORM, : // A small num > This is some particular access pattern, right? Maybe, find a proper word t It was hard to describe it succinctly. I added a description to this pattern that describes the entire access pattern. http://gerrit.cloudera.org:8080/#/c/21601/1/src/kudu/util/cache-bench.cc@136 PS1, Line 136: // Returns a pair of the number of cache hits and lookups. : pair<int64_t, int64_t> DoQueries(const atomic<bool>* done, bool often, uint32_t large_number) { : const BenchSetup& setup = GetParam(); : Random r(GetRandomSeed32()); : int64_t lookups = 0; : int64_t hits = 0; : while (!*done) { : uint32_t int_key; : switch (setup.pattern) { : case BenchSetup::Pattern::ZIPFIAN: : int_key = r.Skewed(Bits::Log2Floor(setup.max_key())); : break; : > Maybe, it's time to rewrite this piece using the 'switch' statement? Done http://gerrit.cloudera.org:8080/#/c/21601/1/src/kudu/util/cache-bench.cc@215 PS1, Line 215: che > Create a constant for this and use it here and at line 142 as well? Done http://gerrit.cloudera.org:8080/#/c/21601/1/src/kudu/util/cache-bench.cc@216 PS1, Line 216: BenchSetup:: > Maybe, move this and large_number_max into BenchSetup, avoid passing on ext That would generate a new large_number for every query rather than having the same large_number for each benchmark. -- 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: 2 Gerrit-Owner: Mahesh Reddy <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <[email protected]> Gerrit-Comment-Date: Wed, 24 Jul 2024 20:41:25 +0000 Gerrit-HasComments: Yes
