refactor(cache): Integrate the Radix LRU cache with the --experimental-enable-optimized-metadata-cache flag#4872
refactor(cache): Integrate the Radix LRU cache with the --experimental-enable-optimized-metadata-cache flag#4872vedantdas-source wants to merge 2 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for an optimized metadata cache using a radix-tree-based LRU cache (RadixLRU). It adds the EnableOptimizedMetadataCache configuration option to BucketConfig and integrates it into BucketManager to conditionally instantiate RadixLRU. It also includes comprehensive memory tests and benchmarks comparing RadixLRU with the standard MapLRU. The feedback suggests stopping the benchmark timer before resetting it in benchmarkErasePrefix to ensure consistent measurements across all iterations.
796ca4a to
4420c9b
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## radix-trie-cache #4872 +/- ##
===================================================
Coverage ? 83.73%
===================================================
Files ? 171
Lines ? 21247
Branches ? 0
===================================================
Hits ? 17792
Misses ? 2784
Partials ? 671
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| runtime.KeepAlive(statCache) | ||
|
|
||
| t.Logf("%-20s Heap Used: %10.2f MB\n", "Map StatCache", float64(pureMem)/(1024*1024)) | ||
|
|
There was a problem hiding this comment.
See if this can be converted to Table driven test or is it better to create 2 independent tests.
Also use AAA.
| } | ||
|
|
||
| i := 0 | ||
| b.ResetTimer() |
There was a problem hiding this comment.
b.Loop() resets the timer automatically on its first call. So we can remove it before b.Loop().
| b.ResetTimer() | ||
| for b.Loop() { | ||
| b.StopTimer() // Timer must be running when b.Loop() evaluates, stop it for setup | ||
| prefix := prefixes[i%len(prefixes)] |
There was a problem hiding this comment.
Since prefix := prefixes[i%len(prefixes)] would take negligible time.
Can we do something like:
i := 0
for b.Loop() {
// 2. TIMED: Erase prefix (Timer is RUNNING from b.Loop)
prefix := prefixes[i%len(prefixes)]
cache.EraseEntriesWithGivenPrefix(prefix)
// 3. UNTIMED: Pause clock, restore erased keys, and restart timer for next iteration
b.StopTimer()
for _, key := range prefixMap[prefix] {
cache.Insert(key, dummyValue{})
}
i++
b.StartTimer() // Timer MUST be running when b.Loop() evaluates next!
}
vipnydav
left a comment
vipnydav
left a comment
There was a problem hiding this comment.
LGTM. Left few minor comments.
2 participants
Description
This PR integrates the Radix Tree based LRU cache with GCSFuse using the
--experimental-enable-optimized-metadata-cacheflag.Changes Made
bucket_manager.goBenchmarking Results
Link to the issue in case of a bug fix.
Testing details
Any backward incompatible change? If so, please explain.