posters: one notification for art the render cache cannot keep resident - #182
Merged
Merged
Conversation
The texture cache and the poster source held two answers to the same question and never reconciled them. When residency crossed TEX_RESIDENT_BYTES_MAX the cache dropped the least-recently-used texture and told nobody; the source's slot stayed P_READY and kept answering "I have this image", while resolve returned absent and the tile drew its skeleton. Nothing put the slot back to P_WANT, so the placeholder stayed until the source LRU happened to recycle that slot for a different key. Six Detail backdrops at 1920x1080 break the ceiling, which is an ordinary evening of browsing. Users report artwork that never loads (#107). The cache's own `failed` set was a second spelling of the same defect, reached through rejection instead of pressure: a decode error put the key there, and it cleared only on a successful accept that no source re-arm could ever produce. So the set is gone and both paths now say one sentence — Source::unresident(key) — drained after the cache borrow is released, so the callback may touch application state without coupling the library half to it. The source parks both in P_EVICTED, dormant until a draw or prefetch asks again: eviction is not demand, and refetching an off-screen key would only evict art somebody is looking at. Pixels are not retained, because a second CPU copy of the whole 44 MiB pool would defeat the ceiling that evicted them. The invariant, which is what the regression test pins: the source never answers READY for a key the cache cannot make resident. 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. |
Owner
Author
|
@codex review Worth being adversarial about two things specifically:
|
![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: e8ab395dd4
ℹ️ 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".
Comment on lines
+507
to
+508
| if g.slots[i].state == P_EVICTED { | ||
| g.slots[i].state = P_WANT; |
There was a problem hiding this comment.
When the live or prefetched working set exceeds the 44 MiB byte ceiling, every draw or warm probe of an evicted key unconditionally moves it back to P_WANT. Its eventual upload then evicts another demanded key, whose next probe performs the same transition, so the keys can alternate through fetch/decode/upload indefinitely. Home's recurring backdrop prefetch also counts as demand here, meaning even off-screen art can participate. The frame budget limits each large upload to one frame but places no bound or cooldown on retries, leaving a steady-pressure screen doing continuous I/O and expensive GL uploads instead of settling.
Useful? React with 👍 / 👎.
…aded The route heartbeat's evicted_hot= counts only HOT evictions, so a device check of issue #107 could report the absence of a skeleton and nothing more. These two counters make the READY->EVICTED eviction and the EVICTED->WANT re-arm separately observable through the event log, throttled to about one line a second because this is a per-frame path and a fast scroll evicts dozens of keys a second. The throttle reads Instant, not app/clock.rs's frame clock, because unresident and lookup both run on worker threads and that clock is main-thread only (per its own module doc) - which makes it an INSTRUMENT under the wall gate, allowlisted the same way the recorder's own timing is. 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: 5989cab712
ℹ️ 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".
Comment on lines
+414
to
+419
| let due = match *last { | ||
| None => true, | ||
| Some(t) => now.duration_since(t) >= Duration::from_secs(1), | ||
| }; | ||
| if !due { | ||
| return; |
There was a problem hiding this comment.
When a single texture is evicted and demanded again within one second, the eviction calls log_residency() first and advances LAST, so the immediately following re-arm is suppressed; if no later residency transition occurs, the only diagnostic line permanently reports rearmed=0. This is the ordinary one-eviction recovery case the new counters are intended to distinguish, so the log can incorrectly suggest that recovery never happened; defer the first emission, emit a trailing snapshot, or allow the paired re-arm through.
Useful? React with 👍 / 👎.
Comment on lines
+383
to
+384
| fn take_unresident(&mut self) -> impl Iterator<Item = K> + '_ { | ||
| self.unresident.drain(..) |
There was a problem hiding this comment.
When a bare TexCache reaches its capacity, evict_for appends a victim to unresident, after which has_pending() remains true until take_unresident() is called. That drain is private to ui::tex, while the existing FixtureRig owns a bare cache and calls TexCache::prepare directly, so its ninth distinct poster leaves the cache permanently pending and can force subsequent fixture frames/present decisions indefinitely. Expose a drain path to direct callers or make prepare consume/report these notifications itself.
Useful? React with 👍 / 👎.
…till drains Codex review on PR #182 (commit 5989cab) found two real defects: 1. The residency log's one-second interval throttle could swallow a re-arm that landed inside the same window as the eviction before it - the ordinary one-eviction-then-recovery case - leaving the last written line permanently stale (lost=N rearmed=0) even though the re-arm counter itself was correct. A TV session read exactly that shape and was reported as "the re-arm branch never fires", which the totals never actually showed. Fixed with a second, settle-triggered emitter (`log_residency_settled`) that bypasses the interval outright and writes whenever the totals moved since the last line - called from `begin_frame`, not `PosterSource::idle`: `idle` only reaches this module through screens/home/mod.rs's prefetch gate (prefetch_armed && source_idle), which is screen-specific and conditionally short-circuited, so it would never fire for an eviction burst on Detail or a library grid. `begin_frame` already runs first, unconditionally, every frame, on the main thread, for every screen - the same seam `invalidate_due_retries` already uses. 2. `ui::fixture::FixtureRig` owns a bare `TexCache` (no `Source`) and calls `TexCache::prepare` directly rather than through `ui::tex`'s free-function wrapper, which is what drains `take_unresident` on the product path. Past its 8-slot cap, `evict_for` queues a notification nothing ever drains, and `has_pending` never reports false again. Fixed with the narrow option: widened `take_unresident`'s visibility to `pub(crate)` rather than changing `prepare`'s return type, since the inherent method's one production caller already pairs it with `take_unresident` in the same statement and a second test-only caller does not justify rippling the signature. A red regression test (a 9th distinct poster through the rig's cap-8 cache) confirmed the defect before the fix and passes after it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codex P1 review on PR #182 (poster.rs:564): lookup's P_EVICTED branch moved a key straight back to P_WANT on every draw or warm probe that found it evicted, unconditionally. A working set that genuinely cannot fit under tex::TEX_RESIDENT_BYTES_MAX (44 MiB) then had the render cache evict key A to make room, A's very next probe re-arm it, A's eventual upload evict key B, B's next probe re-arm IT, and so on with no bound - continuous fetch/decode/upload every frame instead of settling. Home's recurring backdrop prefetch counted as a probe here too, so even off-screen art could drive the cycle. Reproduced with a levelled-down test that drives SOURCE.unresident + a Draw lookup in a tight loop, standing in for the render cache's own byte-pressure eviction (real pressure needs a live GL cache no host test links; the unresident callback IS the seam it calls through): pre-fix, all 20 cycles rearm ("got 20 rearms out of 20 cycles"). Fixed with a per-slot thrash guard: a key's FIRST eviction always re-arms on its very next demand (evicted_at is None, so evict_was_rapid answers false and no cooldown is set at all - this is exactly the ordinary one-eviction-then-recovery case PR #182 itself exists to fix, and it is what the existing a_ready_source_hit_recovers_after_its_ texture_is_evicted test already pins). Only a key evicted AGAIN inside a 10-second thrash window - proof something is still churning it - escalates evict_attempts and pays a bounded cooldown (250ms, 500ms, 1s, 2s, 4s, capped at 8s) before its next re-arm, deliberately its own shorter schedule than retry_backoff's network-outage one. The cooldown applies uniformly to both Draw and Warm touches (not gated by touch type): the two-visible-tiles- alternate-forever case the review describes is two DRAW probes, so a fix that only throttled prefetch would not touch it, and unifying the guard also means it doesn't disturb the warm-probe recovery path the existing regression test exercises. A visible tile can never be starved: the guard is bounded (8s cap) and keyed per-slot, so any key always gets another turn once its cooldown clears - it just does not get one on every single frame while the pressure that evicted it persists. Also found while starting this work: the worktree's committed HEAD (b0c92f4) already carries the P2 fixes for the log-throttle and bare-cache-drain findings, but the working tree for fixture.rs and tex.rs had been reverted to their pre-b0c92f4e content (poster.rs partially so) by an earlier, apparently interrupted session. Restored both files to HEAD before starting this fix; that revert is not part of this commit's diff. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
PR #182's P1 (lookup's P_EVICTED branch): an evicted key that is probed again went straight back to P_WANT regardless of who probed it, so an off-screen prefetch could re-arm a slot the render cache had just discarded, evicting another demanded key in turn - fetch/decode/upload could alternate indefinitely, with Home's recurring backdrop prefetch named as a participant able to push on-screen art out. b0c92f4..0a3561e already answered the DRAW-vs-DRAW half of that finding with a per-slot cooldown that backs off a key re-evicted rapidly; this answers the remaining half, prefetch itself. Only a Touch::Draw probe now even reaches that cooldown gate for an existing P_EVICTED slot. A Touch::Warm probe is turned away before it, unconditionally: it returns Warm::Known and leaves the slot dormant, neither transitioning it, incrementing the re-arm counter, nor consuming a warm claim. Prefetch is opportunistic and cannot protect what it restores, so letting it resurrect a key the render cache just discarded let speculation compete with visible work. The cost is real: an off-screen backdrop that was evicted now only returns when it is drawn, which is a visible pop at the moment of navigation - exactly what prefetch exists to prevent. This is a mitigation, not a complete anti-thrash policy. A DRAWN working set that exceeds the byte ceiling can still cycle - bounded now by the cooldown gate it reaches, not eliminated - and once an evicted slot is recycled by a miss, a later warm can allocate the same key afresh anyway. Rewrote both the branch's inline comment and the thrash guard's module doc, which overclaimed on several points once combined with this fix: the branch comment said "only a real draw or the explicitly quota-gated prefetch path starts... the disk-first replacement", but the quota gate bounds the RATE of speculative requests, not their duration, and "disk-first" overstates coverage, since ordinary server-relative artwork is excluded from the on-disk cache (imgcache.rs); the guard's module doc said a warm probe counts as a probe this guard bounds, which is no longer true - a warm probe never reaches it at all now. Updated a_ready_source_hit_recovers_after_its_texture_is_evicted, whose recovery step used a warm probe to re-arm a dormant slot - that path no longer re-arms, so the test now drives the same recovery with a real draw. Added a_warm_probe_of_an_evicted_slot_leaves_it_dormant, watched red against the pre-fix branch and green after. Checked route/decision.rs:5767 (the session-start next-episode still prefetch): it calls warm_tex_on and discards the returned Warm entirely, so that caller does not enforce the prefetch quota from its return value at all - a separate, out-of-scope finding, but it means the quota bound this comment used to lean on was never as tight as it assumed for every caller. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The adjudication on PR #182 (issue #107) required three things before merge. 1. Nothing previously asked the screen to draw again when an eviction cooldown's deadline passed - re-arming is Draw-only, so on a quiet screen a slot recovered only when something else happened to trigger a frame, later than its own deadline. invalidate_due_evictions mirrors invalidate_due_retries exactly, including the evict_wake_sent latch that keeps one expiry to one redraw request instead of invalidating every frame for a slot nothing draws; the wake only asks for a present, it does not itself re-arm anything. 2. The existing tight-loop thrash test runs on a frozen clock, so it can prove the first cooldown blocks a rearm but nothing about the deadline itself. New tests drive crate::app::clock::set_replay deterministically to cover: a Warm probe dormant on both sides of expiry, a Draw probe refused one tick short and honored at the deadline, repeated completed recoveries escalating through evict_backoff's schedule to its 8s cap, an eviction gap past the 10s thrash window resetting the escalation, and the wake firing exactly once per expiry. 3. Prose corrections for claims the Draw-only gate (b027ed0) and the wake (this commit) made false or overbroad: "very next demand" is now "very next Draw probe" (Warm is excluded); "entirely about the LRU bookkeeping" is false now that touch type also gates recovery eligibility; the "never starved" claim is narrowed to bounded eligibility for another attempt, not successful loading or durable residency; "the cycle the cooldown exists to break" is reworded as a rate limit, since the cycle can continue indefinitely; and the thrash window's "well short of the user browsed away and came back" claim is dropped as unsupported - a user can return inside 10s and pay a cooldown without any sustained overload. 0a3561e's commit message itself still says the cooldown "applies uniformly to both Draw and Warm touches" and "doesn't disturb the warm-probe recovery path" - both obsolete since b027ed0 excluded Warm entirely. That commit message cannot be edited; no doc comment in the current source repeats either claim. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…s no disk tier Two sentences in the module doc went false under this branch. The P_EVICTED lookup branch (b027ed0) now answers a Touch::Warm probe with dormancy before the cooldown gate runs, so a prefetch never revives an evicted key — only a Draw probe does, and the pop stays visible until then. And there is no disk-first fetch path for ordinary artwork: imgcache's disk tier holds only plex.tv avatars (classify/class_of), so an evicted poster, backdrop or hero logo recovers through a full network refetch, decode and upload — the real cost the thrash guard, the cooldown backoff and the Draw-only gate exist to rate-limit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Owner
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
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.
Closes part of #107.
The defect
The texture cache and the poster source held two answers to the same question and never reconciled them.
When residency crossed
TEX_RESIDENT_BYTES_MAX(44 MiB) the cache dropped the least-recently-used texture and told nobody. The source's slot stayedP_READYand kept answering "I have this image", whileresolvereturned absent and the tile drew its skeleton. Nothing put the slot back toP_WANT, so the placeholder stayed until the source LRU happened to recycle that slot for a different key — which, for a tile you keep looking at, is never.A systematicity audit found the same defect wearing a second hat: the cache's own
failedset, reached through rejection instead of pressure. A decode error put the key there, and it cleared only on a successfulacceptthat no source re-arm could ever produce.The change
The
failedset is gone. Eviction and rejection now say one sentence —Source::unresident(key)— drained after the cache borrow is released, so the callback may touch application state without coupling the library half to it. The source parks both cases in a newP_EVICTED.Review then found that re-arming an evicted key was unbounded: under sustained byte pressure key A is evicted, its probe re-arms it, its upload evicts key B, B's probe re-arms B, and so on. Two mitigations answer it, and they cover different halves:
Touch::Drawprobe re-arms. A prefetch probe of an evicted slot returnsWarm::Knownand leaves it dormant — speculation cannot protect what it restores. The cost is real and is not hidden: an off-screen backdrop that lost residency now returns only when it is drawn.Neither makes an oversized visible working set converge, and the prose does not claim otherwise. They bound the rate, not the duration.
Recovery is also not cheap, which is why it is rate-limited:
imgcache's disk tier holds plex.tv avatars only, so an evicted poster pays a full network refetch, decode and upload.The
ui/↔app/adapters/split is unchanged — the cache still knows only an opaque key and never what it denotes.Verification
Gates on the committed tree, working tree empty before and after:
--no-default-features(the shipping set, not covered bymake check): clean.ci/check-deps.sh: all gates green.On the television (rooted webOS 4.5, guest boot, muted), with the two counters this PR adds:
Both halves of the mechanism fire on hardware: eighteen evictions under ordinary navigation, four re-arms. Artwork was complete throughout — no skeleton on any grid tile or detail page at that state.
One measurement worth recording because it invalidates an obvious check: scrolling the poster grid produces no evictions at all.
CACHE_CAPequals the source'sPT_CAP(both 64), so the cache can never hold more keys than the source has slots, and 64 posters is ~24 MB against a 44 MiB ceiling. Byte pressure needs backdrops and hero logos. "I scrolled the library and saw no skeletons" measures nothing here.An earlier revision of this description said six 1920×1080 backdrops break the ceiling; backdrops are requested at 1280×720 (≈3.7 MB), and the pressure comes from backdrops plus hero logos plus the shelves behind them.
🤖 Generated with Claude Code