perf(olmoe): O(1) LRU victim selection via intrusive recency lists (#1050)

[Petsku01]

Summary

Replaces the O(cap) linear scan in the OLMoE expert victim selection with an O(1) intrusive recency list, closing the long-standing #1050 performance issue.

What

• OLMoE expert cache keeps an intrusive doubly-linked recency list threaded through the cached expert entries; eviction picks the LRU tail in O(1)
• No extra allocation — the list pointers live inside the existing entries (intrusive), preserving the current cache layout
• qwen36.c gets the same shared plumbing where the expert-ring layout matches (kept minimal to limit blast radius)

Verification (local, rebased on current dev, gcc -O3 -Wall -Wextra clean)

• tests/test_olmoe_victim_index: ok (222 lines, covers pick order, re-touch recency, eviction tail, cap rollover)
• tests/bench_olmoe_victim_index at cap=219, 2M cycles:
  • list pick: 2.9 ns/cycle
  • legacy scan: 442.6 ns/cycle (~438M steps)
  • speedup: 155.3x
• Full make test suite: OK (skipped=123 — env-gated skips, same set as base dev)

Notes

• Microbenchmark committed separately (first commit) so the number is reproducible independently of the change
• Bench numbers from a WSL2/gcc host; happy to re-run on other targets if useful

Closes #1050

[JustVugg]

Reviewed carefully, and I am asking you to hold this one. The idea is right and the olmoe work is good, but three things need to close first and one of them is a defect I verified in your own diff.

The ordering rule you wrote down is broken at the second site. In expert_get the diff puts the stamp before the refile and carries a comment explaining exactly why: the pin insert-scan has to see the final stamp, and refile-then-bump left a stale-ordered pin list. Then in pilot_realload the same two lines appear in the opposite order, refile first and s->used = ++m->clock second. qwen36.c has it right. Only olmoe.c has it inverted, and the consequence is that the list can pick a different victim from the legacy scan, evicting a recently loaded pinned expert where the scan evicts the genuine least-recently-used one.

The fix is moving one line. What worries me more is that the test did not catch it, and the reason is structural: test_olmoe_victim_index.c calls victim_* and cache_publish/cache_hide directly and never drives expert_get or pilot_realload, so the call sites where the ordering lives are exactly what is not exercised.

colibri.c is not mentioned anywhere in the body. It takes +106/-11 and rewires the victim selection of the GLM streaming engine, which is the most invariant-laden cache in the tree, governed by three prior bug fixes. There are no tests for it at all, and no kill switch: COLI_VICTIM_SCAN works in olmoe.c, is parsed and ignored in qwen36.c, and does not exist in colibri.c. The new behaviour is the default in all three.

That half also looks wrong to me in two ways. eslot_victim_pick tries the free-with-slab counter, then the list head, and never reaches the legacy growth rule that reuses an emptied slot while the row is below capacity, because the list head is non-negative first. And rss_guard hides slots while the slab is still alive, incrementing that counter, then frees the slab without decrementing it. The combination means that after an RSS-guard drop the row permanently evicts live residents instead of regrowing into the slots it just emptied, and the drifted counter sends the picker into a full scan on every pick. On the machine class this project exists for, that is O(n) and a smaller cache, from a patch whose purpose is throughput.

And a smaller one: the NULL check on the three new allocations sits one line below the loop that already dereferences them.

On the performance case. The microbenchmark's two loops are not equivalent, the list one does pick plus hide plus publish plus stamp and the legacy one only picks. But the real issue is scale: this is a per-miss cost, and a miss is a multi-megabyte disk read. Against the roughly eight milliseconds an expert read takes, 440 nanoseconds is about five thousandths of a percent. There is no end-to-end tok/s number anywhere in the PR, and I do not expect one to be measurable.

So the question I would like answered before the rest: what is the end-to-end difference on a real model? If it is not measurable, that is a fine answer and the PR becomes a code-quality change rather than a performance one, which changes how much risk is worth taking in colibri.c.

To make it mergeable: move the refile below the stamp in pilot_realload; move the NULL check above the initialising loop; and split colibri.c into its own PR rather than carrying it unmentioned in this one. Then add a randomised differential test that drives expert_get and pilot_realload through thousands of operations and asserts the list pick equals the legacy scan at every step, per engine. That single test would have caught both defects.

[Petsku01]

All five points addressed on the branch — thank you, this review earned its keep.

1. Ordering rule — fixed (6f4bf70). s->used = ++m->clock now sits before victim_refile in pilot_realload, matching expert_get and qwen36.c.

2. The structural blind spot — fixed, and it caught more. tests/test_olmoe_differential.c drives the real expert_get and pilot_realload call sites (checkpoint load stubbed behind OLMOE_TEST_STUB_LOAD, production build untouched) with randomised ops and random pin flips through the real refile path, asserting list pick == legacy scan on every step. Verified both ways: the fixed tree passes 2×5000 steps; injecting your refile-before-stamp ordering into a scratch copy makes it fail at step 36 with exactly the divergence you described.

Honest note: with pin flips active it also failed on my own corrected code at step 1224 — a pin→ev flip tail-appended the slot on its old stamp, promoting an older resident to MRU over genuinely newer ones. Same defect class as the Sol-r1 M3 pin-list finding, on the ev side. Fixed in the same commit: pin→ev flips now splice by used from the head (rare path, pin-budget bound); fresh-publish keeps the O(1) tail-append because its stamp is provably fresher than every member. The legacy mirror in the test also needed the same two-phase contract as victim_pick (pinned fallback) to avoid a false positive. So the test caught three defects, not one.

3. colibri.c — split into its own commit (9ef81b7) with its own message, so it is no longer unmentioned. All three defects fixed: NULL checks moved above the initialising loop; rss_guard now decrements ecn_freeslab when it frees the hidden slot's slab; eslot_victim_pick no longer lets a non-negative list head shadow the legacy growth rule (nn < ecap goes straight to the legacy scan — the rare path pays the scan, the common path keeps O(1)). One deviation from your suggestion: I tried a separate PR first, but the colibri.c fixes are corrections to the recency-list machinery this PR introduces (they patch de133ca's allocations and pick path), so they cannot stand alone against main without carrying the whole feature. Separate commit, same PR, with the kill-switch question below still open for you.

4. Kill switch — COLI_VICTIM_SCAN is still not wired in colibri.c (0 parse sites vs. 2/2 in the other two). I deliberately did not guess at what a kill switch should do in the streaming engine's three-layer cache and left it as an explicit decision point: happy to add it either as a full legacy-scan fallback or scoped to eslot_victim_pick only, your call.

5. Performance framing — accepting the rebrand. You are right about scale: 440 ns against an ~8 ms disk read is ~0.005% per miss, and the bench loops are not equivalent. The honest answer to "what is the end-to-end difference on a real model" is: not measurable, and I am not going to claim otherwise. If you agree, the PR title/body should be rebranded from perf to code-quality (correctness of the LRU contract under pinning), and the risk bar in colibri.c drops accordingly — happy to shrink the colibri diff to just the NULL-check + rss_guard counter fix and drop the growth-rule rewire if you prefer the conservative side.

CI is running on 9ef81b7. The differential test is in TEST_BINS via Makefile auto-discovery, so it runs in CI from now on.