Conversation
There was a problem hiding this comment.
Pull request overview
This PR adjusts the S3-FIFO cache eviction behavior to better match the intended S3-FIFO policy by refining how items are handled when evicting from the small queue and by adding a second-chance style eviction loop for the main queue.
Changes:
- Change eviction from the small queue to either promote entries with
freq > 0into the main queue or evictfreq == 0entries to the ghost queue. - Update main-queue eviction to use a clock-like “second chance” (decrement
freqand rotate) before evicting whenfreq == 0. - Rename/replace the small-queue eviction helper to reflect the updated policy.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // evictFromMain evicts an item from the main queue using a clock-like second chance. | ||
| // Per S3-FIFO paper: if front item has freq > 0, decrement and reinsert at tail. If freq == 0, evict. | ||
| func (c *S3FIFOCache[K, V]) evictFromMain() bool { | ||
| if c.main.Len() == 0 { | ||
| return false | ||
| } | ||
| for c.main.Len() > 0 { | ||
| e := c.main.Front() | ||
| entry := e.Value | ||
|
|
||
| e := c.main.Front() | ||
| entry := e.Value | ||
| if entry.freq > 0 { | ||
| // Second chance: decrement frequency and move to back | ||
| entry.freq-- | ||
| c.freq[entry.key] = entry.freq | ||
| c.main.MoveToBack(e) | ||
| continue |
There was a problem hiding this comment.
The new clock-like/second-chance logic in evictFromMain changes eviction semantics (front entries with freq>0 are rotated and decremented). There are no tests asserting this behavior (e.g., that a hot entry isn’t immediately evicted and that freq is decremented/moved-to-back until it reaches 0). Please add/adjust unit tests to cover these cases so regressions are caught.
| if entry.freq > 0 { | ||
| // S3-FIFO paper: promote to main if accessed at least once | ||
| entry.queue = 1 | ||
| entry.freq = 0 // Reset frequency on promotion per paper | ||
| newE := c.main.PushBack(entry) | ||
| c.cache[entry.key] = newE | ||
| c.freq[entry.key] = 0 | ||
| } else { |
There was a problem hiding this comment.
evictFromSmall now conditionally promotes the oldest small entry to main when freq>0 (instead of always evicting it to ghost). The current test suite doesn’t appear to cover this branch (freq==1 case) or verify that the entry remains in-cache and ends up in the main queue. Adding a focused test would help validate the updated policy.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #59 +/- ##
==========================================
- Coverage 65.05% 64.86% -0.20%
==========================================
Files 34 34
Lines 3835 3848 +13
==========================================
+ Hits 2495 2496 +1
- Misses 1253 1263 +10
- Partials 87 89 +2
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:
|
No description provided.