feat: Add S4-FIFO algo - #335
Conversation
…ure 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>
…ue 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>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7eaab046d0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return req->obj_size <= params->small_fifo->cache_size && | ||
| cache_can_insert_default(cache, req); |
There was a problem hiding this comment.
When auto-tuning shrinks the small FIFO, an object previously admitted under the larger size can remain in the ghost FIFO while being larger than the new small FIFO. A subsequent ghost hit is supposed to promote that object into the main FIFO, but this check rejects it solely against small_fifo->cache_size, so cache_get_base() never calls S4FIFO_insert() and the promotion is silently lost even when the object fits in the main cache. This is especially likely for the supplied configurations that reduce the small ratio from 10% to 1.5–8%; permit ghost promotions based on their actual destination capacity.
Useful? React with 👍 / 👎.
| int64_t probation_len = | ||
| (int64_t)(params->small_skip_ratio * | ||
| params->small_fifo->get_n_obj(params->small_fifo)); | ||
| if (S4FIFO_small_fifo_age(params, obj) >= probation_len) { | ||
| obj->S4FIFO.freq += 1; |
There was a problem hiding this comment.
Insertion increments small_insert_seq after stamping the object, so resident objects have ages from 1 for the newest through n_obj for the oldest. Using >= probation_len therefore skips only probation_len - 1 objects; for example, small-skip-ratio=1.0 still counts a hit on the oldest object even though the entire small FIFO should be probationary. This changes promotion decisions for the learned configurations, including the shipped class with a 1.00 skip ratio; use a strict boundary or make the age zero-based.
Useful? React with 👍 / 👎.
| if (model->n_classes == 0 || | ||
| model->n_classes > S4FIFO_REAL_MODEL_MAX_CLASSES || | ||
| model->n_models == 0 || model->n_trees_total == 0) { | ||
| WARN( | ||
| "S4FIFO: %s has implausible header (n_models=%u n_classes=%u " | ||
| "n_trees_total=%u)\n", | ||
| path, model->n_models, model->n_classes, model->n_trees_total); | ||
| goto out; |
There was a problem hiding this comment.
When model-path names a magic-valid but incompatible or malformed .s4m, this validation accepts it without checking that estimator counts match n_trees_total, feature indices fit the 76-element input, or child/tree offsets stay within their arrays. eval_tree() then trusts those values and can read past trees, nodes, leaves, or the input buffer, causing a crash or corrupted prediction instead of the documented fallback to the embedded model. Validate the complete structure before publishing the loaded model.
Useful? React with 👍 / 👎.
1 participant
S4-FIFO generalizes S3-FIFO with two extra tunable parameters. Build with
-DENABLE_S4FIFO=ON. #332Usage:
Results
Example 1
10M-request twitter trace, whole trace scored, miss-ratio reduction vs FIFO:
Example 2