Repository navigation
feat: Add S4-FIFO algo - #341
Merged
Merged
Conversation
* feat: Add S4-FIFO algo * fix(s4fifo): address review findings on child sizing, warmup and feature tracking Five fixes from the review of the S4-FIFO PR: - Size each sub-FIFO's hash table from its own byte size, capped by any requested --hashpower, rather than inheriting the parent's. Previously a single S4FIFO allocated three full-size tables; at a 1GiB cache size peak RSS drops from 527MiB to 338MiB, matching S3FIFO's 340MiB, with an identical miss ratio. - Leave WARMUP on the first capacity eviction instead of when occupancy reaches cache_size. Eviction runs before occupancy can exceed the limit, so the old test needed the trace to fill the cache exactly and in practice never fired: on cloudPhysicsIO the learned mode spent the whole trace in warmup and never predicted a configuration. - Only pre-scan the trace for its request count when an algorithm actually reads it. Every cachesim and MRC run paid for a full extra pass, which on a txt or zstd trace costs as much as the simulation (LRU on a 1M-request csv: 0.48s -> 0.33s). - Stop double-counting one-hit-wonders: S4FIFO_track_ghost_insert already records them, so the extra call doubled one_hit_count whenever a ghost FIFO was configured. - Stamp clock reinsertions in the main FIFO with S4FIFO_track_main_insert. A reinserted object takes a fresh tail position but kept a stale insert_seq, skewing the main queue's hit-position histogram. The eviction path itself is unchanged: the static heuristic still matches S3-FIFO exactly, and the full test suite passes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(s4fifo): propagate n_total_req through cloning, and fix ghost-queue tracking Second round of review fixes. - Adding n_total_req grew common_cache_params_t from 24 to 32 bytes, but clone_cache and create_cache_with_new_size still asserted the old size, so any assertion-enabled build aborted in simulate_at_multi_sizes (testSimulator exited 134 under -DCMAKE_BUILD_TYPE=Debug). That assert was a tripwire for exactly the bug that had happened - both functions rebuild the struct field by field and were silently dropping the new field - so copy n_total_req in both, retain it on cache_t to copy it from, and make the tripwire a _Static_assert. As a runtime assert it was compiled out of release builds and so never fired in CI; now it breaks the build for everyone, which is the point. Verified it still trips by adding a scratch field. - MINISIM hardcoded n_total_req = 0 even though it had just counted the trace, so the default fractional feature-collect-reqs fell back to a fixed 10000 and MRC disagreed with cachesim. Pass sampled_cnt - not n_req_, since spatial sampling drops requests before the cache sees them, so sampled_cnt is what cache->n_req climbs to. On twitter_cluster52 at wss 0.055 the miss ratio goes 0.216370 -> 0.205729, and the curve now matches cachesim exactly (0.299438 and 0.185243 against 0.2994 and 0.1852). - S4FIFO_track_ghost_insert returned early outside a collection window, so objects inserted during warmup or between windows kept a zero insert_seq and ghost_insert_seq froze. A later hit inside a window was then scored against a stamp from a different sequence. Always stamp and advance, as the small and main helpers do, and gate only the per-window one-hit counter. Costs ~1% throughput (4.82 -> 4.78 MQPS) for the extra lookup. - record_ghost_removal ran on every ghost hit, but with a positive ghost-to-main-threshold the entry deliberately stays in the queue. The tracker subtracts recorded removals as holes from later hit positions, so this invented holes that were never there; record it only in the branch that actually removes. Release and Debug both build clean and pass all 10 tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (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
2 participants
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.
S4-FIFO generalizes S3-FIFO with two extra tunable parameters. Build with
-DENABLE_S4FIFO=ON. #332This PR brings the implementation #335 to the main branch.
Usage:
Results
Example 1
10M-request twitter trace, whole trace scored, miss-ratio reduction vs FIFO:
Example 2