Posters: a fast scroll stops asking for art it does not already have - #187
Merged
Merged
Conversation
…ndex The poster worker pool used to claim the lowest-index pending slot, which is a priority inversion the moment claims have more than one origin: a speculative warm queued into a low slot could be picked ahead of a visible tile a draw is actually waiting on, purely by index luck. Each slot now carries a monotonic visible bit set the moment any draw touches its key, and workers scan for a visible P_WANT slot before falling back to a speculative one. Speculative admission is refused outright while any visible request is outstanding, and even in a quiet queue it is capped at PREFETCH_OUTSTANDING_MAX = 1, so speculation can never occupy both workers and a late-arriving visible miss always finds a worker free. The tile-lookahead / prefetch feature that originally exposed this ordering bug is NOT part of this change and does not land: it doubled render-thread texture uploads during a continuous scroll and dropped frames against the scrolling baseline on the television. With no lookahead consumer, every claim in this tree still originates from a visible tile or from the existing speculative call sites (hero backdrop art, hero logo, an up-next episode thumbnail), so behaviour here is unchanged from before except for the ordering guarantee itself. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A 1000-movie library dropped ~95 frames per 18s sweep of `fps:library-scroll` and
ran at 55 fps, where a 37-movie library held 60 with 11. The whole difference is
poster work: the small library reports `budget=0/0` through the sweep because
everything it needs is already resident, the large one reports `budget=15/12`
every heartbeat and 72 of its 97 dropped frames carry a texture upload.
The cost is not on the CPU, which is why the obvious fixes did nothing. `diag::spans`
reads `prep:0.0` on the very frames whose FRAMEDROP `prepare=` field says 7 — that
span wraps `prepare_pass`, which contains the GL upload. What moves instead is
`clearx2`, from 0.2 ms to 7-22 ms: the CPU blocking on a GPU that is behind. An
upload is cheap to issue and expensive to have issued, and the span profile is the
same SHAPE at 37 movies as at 1000. Only the number of frames that miss differs.
So the lever is not a bigger cache and not a cheaper upload path, it is not asking.
A draw that misses while the document is scrolling faster than the shared threshold
now claims no slot: no worker, no fetch, no decode, no upload. The demand is
deferred rather than dropped — nothing about the miss is recorded, so the next frame
asks again and art resolves as the view slows. That is also what the reference
clients do; it is not a concession.
Measured on the dev set, 1000-movie mock, panel off, two runs each:
drops median fps
a9d4e8a, no gate 90, 97 55
slot + cache caps 64 -> 160 226, 231 50
rate-limit the uploads after decode 172, 156 53, 55
allow ONE visible request in flight 204, 196 55, 54
decline the request outright 21, 23 60
and on the real 37-movie dev library, 13 drops at 60 fps against 11 ungated — the
small case is untouched, as it must be, since it never reaches the gate.
The three failed rows are kept as branches rather than prose: `probe-slot-cap` and
`probe-upload-rate-limit` carry their own numbers in their commit messages. The
informative one is the fourth: allowing a single visible request in flight paced the
pipeline perfectly (`budget=` 15-18 admitted, ZERO refused, against 15-18 admitted
and 11-15 refused ungated) and still cost twice the baseline's dropped frames.
Pacing the work does not make it affordable.
`ui::card_row` owns the one motion signal, reset per iteration and written by every
scrolling axis through `Spring::step_scroll`, so a still child cannot overrule a
moving document and no consumer keeps a second opinion beside it.
Co-Authored-By: Claude Opus 5 <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: ce48077fe7
ℹ️ 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".
Review of #187 found the gate in two places it did not reach, and both were found independently by the Codex connector and the doc audit. A shelf's own horizontal scroll did not report into the motion signal. `CardRow::update` stepped `scroll_x` with plain `Spring::step`, so walking focus along a Home row moved the shelf at 1805 px/s -- fifteen times the threshold -- while `scrolling_fast()` read false, because the grid's vertical spring was sitting at its target. That is the one axis that reveals a screenful of new art with no document-level motion at all, so the gate slept through precisely the reveal it was built for. The gate also guarded only the fresh miss, not the two re-arm transitions in the matching-slot branch. Those matter more than the miss rather than less: a fast scroll is what evicts textures, so scrolling back across the same rows finds `P_EVICTED` slots -- and a first eviction sets no cooldown, so each one re-armed a full fetch, decode and upload through a branch that returns long before the miss path is reached. A due `P_RETRY` re-queued the same way. The decision is now taken once, before the slot loop, and every initiation point answers from it; a hit is deliberately untouched, so art that is already resident still draws and still takes its LRU touch. Three regression tests, each observed red first: the evicted slot re-armed to P_WANT under a fast scroll (1 against 8), the due retry re-queued (1 against 7), and the shelf reporting nothing while travelling at 1805 px/s. Also corrects the prose the change falsified: `PREFETCH_MAX_SCROLL_PX_S` now names which axes report and says the rail, tab strips and text panels are excluded deliberately (they animate over a still page and carry no art); `tex::Source::probe` no longer promises that a miss always starts a fetch, since `None` can now mean deferred; `lookup`'s doc no longer calls `touch` the only difference between the paths; and the scheduler design record no longer tells the next implementer to preserve an admission rule this branch removed, or to A/B a queue ordering against a baseline the gate has emptied. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5113894319
ℹ️ 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".
GLinnik21
added a commit
that referenced
this pull request
Sep 21, 2026
Follow-up to #187, which gated poster requests on a shared fast-scroll signal. Three review findings, all the same shape: a motion source the signal never saw. The hero-to-grid dive is the largest reveal of poster art the application performs, and it reported nothing. `snap` travels 0 -> 1, so the spring's own velocity is a couple of FRACTIONS per second, while `layout_grid` turns that fraction into `GRID_TOP_Y - PEEK_Y` = 617 px of vertical travel for every shelf. Both `grid.scroll_y` and every `scroll_x` can hold perfectly still through it. Reporting `snap.vel` would not have fixed it -- the units are wrong -- so what is reported is the displacement the layout actually realises. The dive scales the shelves' retained HORIZONTAL offsets by the same fraction: `Grid::eff_scroll` is `scroll_x * snap`, so returning to the hero from the far right of a row sweeps its cards that offset across the screen while the shelf spring is stationary. The axes do not settle together -- at snap 0.9 the vertical term measures 88 px/s, already under the threshold, while a 4000 px offset swept by the same step is ~570 px/s. Both are reported and `note_scroll` keeps the larger, so the gate answers from the document's fastest axis. Between those two, sharing one expression between `layout_grid` and the motion report turned out to be unsafe: it reassociated `(top + flow) - scroll * snap` into `(top - scroll * snap) + flow`, which differs in f32 for 26% of realistic triples, and `base_y` is serialised into `LogicalState` through `CardRow::write_motion` -- so every committed replay recording that scrolls the Home grid would have hash-diverged. `layout_grid` is byte-identical to main again; `grid_origin` serves the motion report alone and the two are pinned by a bit-exact test at row 0, where `flow` is 0.0 and the forms genuinely agree. Worth recording: nothing replays those recordings. `tests/test_harness.py` uses the fixture NAMES against a fake simulator in a temp dir, so `make check` and all five CI checks passed on the divergence. A LogicalState change is invisible to every gate until someone re-records. Audited and deliberately left alone: `hero_slide` is a 0..1 fraction times `SCR_W`, 1920 px, and looks exactly like the `snap` bug -- but its consumers move only the backdrop, hero content and hero hit rects, while grid cards are placed from `base_y` and `eff_scroll`, so a flip creates no new tile demand. Three host tests, each observed red first. Not measured on the television: no fps scene drives a hero-to-grid dive, so this is host-tested only. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Sep 21, 2026
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.
A 1000-movie library dropped ~95 frames per 18 s sweep of
fps:library-scrolland ran at55 fps, where the 37-movie dev library held 60 with 11 drops. This makes the large library
behave like the small one.
What the cost actually is
Not the CPU, which is why the obvious fixes did nothing.
diag::spansreadsprep:0.0onthe very frames whose FRAMEDROP
prepare=field says 7 — that span wrapsprepare_pass,which contains the GL upload. What moves instead is
clearx2, 0.2 ms → 7-22 ms: the CPUblocking on a GPU that is behind. A poster upload is cheap to issue and expensive to have
issued, because
warm_texforces the tile conversion and the GPU pays for it later.The span profile is the same shape at 37 movies as at 1000. Only the number of frames
that miss differs — 1% against 9%.
So the lever is not a bigger cache and not a cheaper upload path. It is not asking.
The change
A draw that misses while the document scrolls faster than the shared threshold claims no
slot: no worker, no fetch, no decode, no upload. The demand is deferred rather than dropped —
nothing about the miss is recorded, so the next frame asks again and art resolves as the view
slows. That is also what the reference clients do.
ui::card_rowowns one motion signal, reset per iteration, so a still child cannot overrule amoving document and no consumer keeps a second opinion beside it. Every axis that moves a
document carrying art writes to it: the page scrolls through
Spring::step_scroll, a shelf'sown horizontal
scroll_x, and Search, which drives its scroll throughmotion::springand soreports by hand. Chrome deliberately does not — the A-Z rail, the tab strips and the text
panels draw no art and animate over a page that is holding still, so letting one of them raise
the signal would decline posters for a document that is not moving.
Device measurements
Dev set, 1000-movie mock library, panel off, two runs each, dropped frames after warmup:
a9d4e8a2, no gateprobe-slot-cap)probe-upload-rate-limit)Real 37-movie dev library: 13 drops at 60 fps against 11 ungated — the small case never
reaches the gate and is untouched.
The two failed directions are pushed as branches rather than described, so the numbers are
readable without re-running them. The informative row is the fourth: one request in flight
paced the pipeline perfectly (
budget=15-18 admitted, zero refused, against 15-18admitted and 11-15 refused ungated) and still cost twice the baseline's dropped frames.
Pacing the work does not make it affordable.
One caveat that should travel with the 60 fps
fps:library-scroll's oscillator reverses at the document's own ends and never settles, sounder this gate that scene does no poster work at all (
budget=0/0for the full 18 s). Thefps figure therefore partly measures work not done, and is not by itself proof of a product
improvement. The product evidence is separate: stepping 18 rows into a 1000-item grid leaves
the grid fully populated, and the real library's numbers are unchanged.
Review round (2/2 Codex P2s, both real)
Codex and the
doc-claim-auditorindependently found the same hole, and Codex found a second.Both are fixed in
51138943, each with a regression test observed red first.A shelf did not report.
CardRow::updatesteppedscroll_xwith plainSpring::step, sowalking focus along a Home row moved the shelf at 1805 px/s — fifteen times the threshold —
while
scrolling_fast()read false, because the grid's vertical spring was parked at itstarget. That is the one axis that reveals a screenful of art with no document-level motion at
all, so the gate slept through precisely the reveal it was built for.
The gate guarded only the fresh miss. The two re-arm transitions in the matching-slot branch
return long before it. Those matter more than the miss, not less: a fast scroll is what evicts
textures, so scrolling back across the same rows finds
P_EVICTEDslots — and a first evictionsets no cooldown — so each one re-armed a full fetch, decode and upload. A due
P_RETRYre-queued the same way. The decision is now taken once, before the slot loop, and every
initiation point answers from it. A hit is deliberately untouched: resident art still draws and
still takes its LRU touch, so declining never blanks a tile that had its picture.
The doc audit's other three findings were prose this change falsified, corrected in the same
commit:
tex::Source::probeno longer promises that a miss always starts a fetch (Nonecannow mean deferred),
lookup's doc no longer callstouchthe only difference between thepaths, and
docs/poster-scheduler-design.mdno longer tells the next implementer to preservean admission rule this branch removes, or to A/B a queue ordering against a baseline the gate
has emptied.
Verification
make checkgreen on the committed tree;cargo test --lib4024 passed; 0 failed; 1 ignoredcargo check --lib --no-default-featurescleanmakeARM cross-build linksa_fast_scroll_declines_a_visible_miss_and_a_settle_takes_it,a_fast_scroll_declines_to_re_arm_an_evicted_slot(re-armed toP_WANT1 againstP_EVICTED8),a_fast_scroll_declines_to_re_queue_a_due_retry(1 againstP_RETRY7), anda_shelf_reports_its_own_horizontal_scroll_to_the_motion_signal(silent at 1805 px/s)ce48077f. The twofixes only ever widen where the gate applies, so they cannot move those numbers upward,
but the re-scroll-over-evicted-rows case they repair is not one
fps:library-scrolldrives —it is unmeasured on the set, and I would not claim otherwise.
🤖 Generated with Claude Code