Fix CLFUS RAM cache value metric broken by integer division#13233
Open
phongn wants to merge 1 commit into
Open
Fix CLFUS RAM cache value metric broken by integer division#13233phongn wants to merge 1 commit into
phongn wants to merge 1 commit into
Conversation
PR apache#11733 rewrote the CACHE_VALUE_HITS_SIZE cast so static_cast<float> wraps the whole quotient, making (hits + 1) / (size + overhead) integer division. It truncates to 0 for normal object sizes, zeroing the value metric and collapsing CLFUS to FIFO: no promote-on-hit, no clock second chance, and no value-based ghost re-admission. Bind the cast to the numerator to restore floating-point division, and add the ram_cache_clfus_value regression test as a guard (it fails on the pre-fix macro and passes after). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
The CLFUS RAM cache ranks objects by a value density,
value = (hits + 1) / (size + overhead), compared against a runningdoubleaverage on every eviction/admission decision. Since #11733 this has been computed with integer division and truncates to0for essentially every object, silently disabling CLFUS's frequency/size/recency logic and degrading it to FIFO.Root cause
#11733 ("Fix C-style cast in Cache subsystem") modernized the macro:
The original C cast bound only to the numerator, so the division was floating point. The
static_cast<float>(...)wraps the whole quotient, so(hits + 1) / (size + overhead)is now integer division.hitsis on the order of tens whilesize + overheadis thousands+, so the result truncates to0for any normal object.With
value ≡ 0,_average_valuestays0and every value-gated branch is dead (0 > 0is false):get()),put()),CLFUS effectively becomes FIFO, which generally performs worse than the simpler LRU it is meant to beat. This has affected every CLFUS deployment since the regression merged on 2024-08-28.
Fix
Bind the cast to the numerator only, restoring floating-point division (the
pre-#11733 semantics):
Test
Adds
ram_cache_clfus_value, a focused regression test asserting the value metric is a non-zero fraction and is monotonic in hits and in size. It fails on the pre-fix macro and passes after:FAILED("value metric truncated to zero", etc.)PASSEDThe existing
ram_cacheregression still passes. With the fix, CLFUS's size-aware behavior is restored — in the bundledram_cachetest at the 16 MB size, its variable-size hit rate rises to ~0.82, slightly ahead of LRU (~0.80) on the same workload.Notes
proxy.config.cache.ram_cache.algorithm = 0(CLFUS) since Fix C-style cast in Cache subsystem #11733; a backport may be warranted.Fixes a regression introduced in #11733.