Conversation
LRU ran the seen filter before looking the key up, so a put for a resident entry, and a put replacing a stale copy of the key under a new auxkey (a rewrite), were each declined once: the first loses its recency bump, the second costs a second disk read. Look the key up first and filter only keys the cache does not hold, which is the result CLFUS's persistent fingerprint already gives. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The focused changes have targeted regression coverage and no unresolved blocking issues.
Pull request overview
Updates LRU RAM-cache admission so the seen filter applies only to genuinely new keys.
Changes:
- Reorders lookup before seen-filter evaluation.
- Bypasses filtering for resident entries and rewrites.
- Adds and registers focused regression tests.
File summaries
| File | Summary |
|---|---|
src/iocore/cache/unit_tests/test_RamCacheSeenFilter.cc |
Adds coverage for filtering, rewrites, resident puts, and recency. |
src/iocore/cache/RamCacheLRU.cc |
Corrects seen-filter ordering and replacement handling. |
src/iocore/cache/CMakeLists.txt |
Registers the new unit-test target. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
RamCacheLRU::putevaluated the seen filter (proxy.config.cache.ram_cache.use_seen_filter, on by default) before looking the key up. The filter is a per-slot bit: first sighting sets it and declines, second sighting clears it and admits. So any put for a key the cache already held met a cleared slot and was declined once. Two kinds of put fall into that:fixup). The next read misses on the auxkey, goes to disk, and puts(K, new_off). The walk discarded the stale copy, then the filter declined the put — so every rewrite of a hot object cost two disk reads before it was RAM-resident again. This is the routine production case.!doc_from_ram_cache, so this arises only in the concurrent-miss race, but the put is a reference and should not be turned away.CLFUS has neither problem: it gates its filter on
!e, and its filter is a persistent fingerprint (_seen[s] == k) rather than a toggle, so a replaced key matches and is admitted at once. S3-FIFO has no seen filter.Split out of #13380, where it surfaced; it is pre-existing LRU behavior and independent of the
copywork there.Change
Look the key up first. A same-auxkey match bumps recency and returns, as before. An auxkey conflict removes the stale entry and sets
replaced; the filter then runs only for keys with no entry at all. Three lines of logic and a comment.Notes for reviewers
seen[j]. Under the old order a resident put alternately set and cleared the slot, so an evicted hot object was re-admitted on its first put about half the time, and unrelated keys sharing the slot saw the flips. Now the slot is always clear after admission and every evicted object needs two puts to return. That is the documented semantics and matches the other policies; the toggle-on-resident-put was incidental to Improve seen list for the RAM cache #10662._destroy-then-filter order: the stale copy is removed even in the (now unreachable) case where the filter would have declined, and in threshold mode (use_seen_filter > 1) the fill check readsbytesafter the stale copy has been reclaimed, i.e. actual occupancy.use_seen_filteris read once at startup and both LRU and CLFUS size their filter state ininit(), which is why the test sets it before constructing the cache.Tests
New
test_RamCacheSeenFilter.cc, four cases, 57 assertions:putreturns 0 then 1) so the case fails if the filter is not engaged, then two resident puts both return 1.(K,1)admitted,put(K,2)returns 1 immediately,get(K,1)misses,get(K,2)hits.ram_cache_bytesgauge, cache filled to derived capacity, first object re-put, one more admitted, the second-oldest is the victim.Each piece is mutation-verified:
replacedskip disabled (filter still runs for replaced keys)REQUIRE(put == 0)Follow-ups (not in this PR)
test_RamCacheCompressEntries.cccarries its own copies of these fixtures; hoist them intotest_doubles.h.seen.resize(size * 2)and% (nbuckets * 2)areintarithmetic and overflow for the lastbucket_sizes[]entry (needs ~257 GB of RAM cache in one stripe).🤖 Generated with Claude Code