Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #60 +/- ##
==========================================
+ Coverage 64.86% 65.56% +0.70%
==========================================
Files 34 34
Lines 3848 3848
==========================================
+ Hits 2496 2523 +27
+ Misses 1263 1241 -22
+ Partials 89 84 -5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Expands S3FIFO’s test suite to cover additional eviction edge cases, and includes a couple of small benchmark-related fixes.
Changes:
- Add new S3FIFO tests covering small-queue promotion, ghost eviction, and main-queue second-chance behaviors.
- Fix
wrappedCache.Purge()to actually forward to the underlying cache’sPurge(). - Correct the v2 rand parallel benchmarks to call
randv2functions.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/s3fifo/s3fifo_test.go | Adds multiple new edge-case tests for small/main eviction and second-chance behavior. |
| bench/wrapper_mock.go | Fixes wrapper Purge() to call the proper underlying method. |
| bench/devel_test.go | Updates rand v2 parallel benchmarks to use randv2 APIs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Fill cache to capacity to trigger eviction | ||
| cache.Set("b", 2) // small: [a(1), b(0)], no eviction yet (Len=2 < 3) | ||
| cache.Set("c", 3) // small: [a(1), b(0), c(0)], Len=3 >= 3 → evict | ||
| // evictFromSmall: front is "a" with freq=1 > 0 → promote to main | ||
| // Then evictFromSmall again: front is "b" with freq=0 → evict to ghost | ||
| // Then insert "c" → but "c" was already set... let me reconsider | ||
|
|
||
| // Actually Set("c") triggers eviction before inserting "c": | ||
| // small: [a(1), b(0)], Len=2. Wait, Len=2 < capacity=3, no eviction for "c". | ||
| // After Set("c"): small: [a(1), b(0), c(0)], Len=3 | ||
|
|
||
| // Now Set("d") triggers eviction (Len=3 >= 3) | ||
| cache.Set("d", 4) | ||
| // evictFromSmall: front "a" has freq=1 > 0 → promoted to main with freq=0 | ||
| // Still Len=3 >= 3, evictFromSmall again: front "b" has freq=0 → evict to ghost | ||
| // Now Len=2 < 3, insert "d" |
There was a problem hiding this comment.
The inline commentary around Set("c") is internally inconsistent (it first claims eviction occurs at Len==capacity, then corrects itself) and includes conversational text ("let me reconsider"). Please rewrite/remove this block so the comments accurately reflect Set’s eviction timing (eviction happens before insertion when Len() >= capacity) and keep the test narrative concise.
| // Fill cache to capacity to trigger eviction | |
| cache.Set("b", 2) // small: [a(1), b(0)], no eviction yet (Len=2 < 3) | |
| cache.Set("c", 3) // small: [a(1), b(0), c(0)], Len=3 >= 3 → evict | |
| // evictFromSmall: front is "a" with freq=1 > 0 → promote to main | |
| // Then evictFromSmall again: front is "b" with freq=0 → evict to ghost | |
| // Then insert "c" → but "c" was already set... let me reconsider | |
| // Actually Set("c") triggers eviction before inserting "c": | |
| // small: [a(1), b(0)], Len=2. Wait, Len=2 < capacity=3, no eviction for "c". | |
| // After Set("c"): small: [a(1), b(0), c(0)], Len=3 | |
| // Now Set("d") triggers eviction (Len=3 >= 3) | |
| cache.Set("d", 4) | |
| // evictFromSmall: front "a" has freq=1 > 0 → promoted to main with freq=0 | |
| // Still Len=3 >= 3, evictFromSmall again: front "b" has freq=0 → evict to ghost | |
| // Now Len=2 < 3, insert "d" | |
| // Fill cache close to capacity; eviction is checked before insertion when Len() >= capacity | |
| cache.Set("b", 2) // small: [a(1), b(0)], Len=2 < 3 → no eviction | |
| cache.Set("c", 3) // small: [a(1), b(0), c(0)], Len was 2 before insert → still no eviction | |
| // Now Set("d") sees Len==capacity before insertion and triggers eviction | |
| cache.Set("d", 4) | |
| // evictFromSmall: front "a" has freq=1 > 0 → promoted to main with freq=0 | |
| // Still Len>=capacity, evictFromSmall again: front "b" has freq=0 → evicted to ghost | |
| // After evictions Len<capacity, then "d" is inserted into small |
| func (c *wrappedCache[K, V]) Purge() { | ||
| c.InMemoryCache.Len() | ||
| c.InMemoryCache.Purge() |
There was a problem hiding this comment.
PR title indicates only test(s3fifo) changes, but this PR also modifies benchmark helpers (bench/wrapper_mock.go and bench/devel_test.go). Consider updating the title/description to mention benchmark fixes as well, or splitting these into a separate PR for clearer history.
No description provided.