Wire ImageCache size-variant LRU eviction to fix unbounded memory growth - #15170
Wire ImageCache size-variant LRU eviction to fix unbounded memory growth#15170warp-agent-staging[bot] wants to merge 2 commits into
Conversation
Fixes unbounded ImageCache growth from unevicted resized/decoded image size variants. Previously, every render of an asset at a new size/fit (e.g. dragging to resize a window with an image in the Markdown viewer) inserted a fresh decoded+resized Arc<StaticImage> into the per-asset RenderedImageCache map, and nothing ever evicted old size variants. evict_size() already existed (Prep 1, APP-3877) but was never called from production code, so it carried #[allow(dead_code)]. This wires it into a lazy LRU eviction pass inside ImageCache::image(): - Each cached size variant now tracks a last_accessed sequence number, bumped on every hit or insert (RenderedCacheEntry). - Each asset retains at most MAX_CACHED_SIZES_PER_ASSET (8) size variants. On a cache miss once an asset is at capacity, the least-recently-used variant is evicted via evict_size() before the new one is inserted. - The eviction check only scans the handful of entries for the one asset being requested, not the whole cache, and the common (hit / miss-below-capacity) paths still use a single upgradable-read-then-upgrade lock cycle, matching the prior behavior. Only the rare at-capacity path pays an extra lock cycle, which is negligible next to the CPU decode/resize it's paired with. Removed the #[allow(dead_code)] and TODO(APP-3877) from evict_size now that it's genuinely exercised by production code. Added tests verifying: the per-asset cache stays bounded at MAX_CACHED_SIZES_PER_ASSET under a simulated continuous resize (with evicted variants' Arc<StaticImage> strong counts reaching zero), and that legitimately rendering an asset at several fixed sizes at once does not thrash (no eviction/recompute) as long as the count stays at or under the cap. Co-Authored-By: Warp <agent@warp.dev>
warp-agent-staging
Bot
added
area:performance:memory
|
This PR was generated with Warp.   |
- test_image_does_not_evict_active_size_variants_at_capacity now proves that cache hits refresh LRU recency, not just that a steady set of MAX_CACHED_SIZES_PER_ASSET sizes doesn't thrash: it re-hits the oldest entry, forces an eviction with a new size, and asserts the refreshed entry survives while the true next-oldest entry is evicted. Verified this fails if the hit-path recency bump in image() is removed. - evict_size's doc comment no longer enumerates its caller; the behavior/cascade contract above it already carries that meaning. Co-Authored-By: Warp <agent@warp.dev>
![warp-agent-staging[bot]](https://avatars.githubusercontent.com/in/2196476?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
Wires the per-asset LRU eviction pass into ImageCache::image(), bounding rendered size variants at MAX_CACHED_SIZES_PER_ASSET and activating the evict_size() that Prep 1 left dead. The bound holds on every insertion path traced (resized BySize, SVG intrinsic rasterization, and Original when max_dimension forces a resize), the evict_size() call passes resolved cache-key values, and the extra unlock/relock is not a race or deadlock hazard because ImageCache is Rc-backed and foreground-owned; one open question on the chosen cap is left inline for a human decision.
Verdict
Checks: build pass, tests pass (315 passed, 7 skipped in warpui_core), CI green (no failing checks; heavy jobs skipped while draft), visual proof n/a
Found: 0 critical, 0 important, 0 suggestions, 0 nits, 1 question
Responding as wilson: Open session · View factory task
| /// UI, or theme picker previews rendered at different form factors). Once an | ||
| /// asset has this many cached variants, requesting a new size evicts the | ||
| /// least-recently-used one first (see the lazy eviction pass in `image()`). | ||
| const MAX_CACHED_SIZES_PER_ASSET: usize = 8; |
There was a problem hiding this comment.
question: A cap of 8 turns any surface that renders 9+ concurrent sizes of one asset into a decode-per-frame treadmill. With a stable 9-size set requested each frame, the 9th request evicts the size needed first on the next frame, and every request from then on misses and re-decodes — worse than the leak this fixes. Either name the largest active cardinality you want supported and raise the cap to match, or make the pass frame-aware so it never evicts a variant used in the current frame; a regression test at N = cap + 1 should follow whichever you pick.
Description
Fixes unbounded
ImageCachegrowth from unevicted resized/decoded image size variants (APP-3877).Root cause (confirmed via heap profile analysis of a production Sentry event, ~9.2 GB sampled heap, 77% attributed to
zune_jpeg::upsampler::scalar::upsample_vertical):ImageCachestores aRenderedImageCachekeyed by(asset hash, RenderedImageCacheKey{bounds, fit_type, animated_image_behavior}). Every time an image is rendered at a new size/fit (e.g. dragging to resize a window with an image in the Markdown viewer, or any UI that renders the same asset at varying dimensions), a fresh decoded+resizedArc<StaticImage>is inserted into this map, but nothing ever evicted old size variants, so the map grew unbounded for the lifetime of the app.Prep 1 (already merged) added a private
evict_size()method, marked#[allow(dead_code)]with aTODO(APP-3877), because the "main changeset" that wires it intoImageCache::image()had not landed. This PR implements that wiring.Eviction policy
Bounded per-asset LRU cap (
MAX_CACHED_SIZES_PER_ASSET = 8): each asset retains at most 8 distinct rendered size variants. Every cached entry now tracks alast_accessedsequence number, bumped on every cache hit or insert. When a cache miss occurs and the asset is already at capacity,image()'s lazy eviction pass finds the least-recently-used variant among that asset's (at most 8) entries and evicts it viaevict_size()before inserting the new one.Why this policy: it directly bounds memory (at most 8 resized copies per live asset, regardless of how many distinct sizes have ever been requested over the asset's lifetime — e.g. during a long window-resize drag), requires no new per-frame hooks (no
end_frame/tick plumbing throughcore/app.rs), and only ever scans the handful of entries belonging to one asset, never the whole cache. I considered a frame-based approach (mirroringTextureCache::end_frame(), evicting anything unused for N frames) but rejected it here: it requires wiring a new per-frame callback throughcore/app.rs/windowing, and does an eviction scan over the entire cache every frame rather than a bounded, lazy, per-asset check only when a genuinely new size is requested. (I reviewed the two stale, unmerged community attempts at this — warp-external#556 "frame-based pruning" and warp-external#554 "LRU cache cap" — for ideas; both are spec-only with no code. The LRU cap approach here follows warp-external#554's spec.)Memory bound guaranteed: the per-asset size-variant map can never hold more than 8 entries, so worst-case retained memory for size variants of one asset is bounded by
8 * (largest requested size in bytes), instead of growing with the number of distinct sizes ever requested.Locking: the hot hit-path and the common miss-below-capacity path are unchanged from before — a single
upgradable_read→upgradelock cycle, no extra acquisitions, and no O(n) scan of the whole cache. Only the rare "asset at capacity, new size requested" branch pays one extra lock cycle (evict_sizeacquires its own write lock, since it can't be called while already holding the write guard from a would-beupgrade—parking_lot::RwLockis not reentrant). That branch already pays for a full CPU image decode/resize, so the extra lock cycle is negligible in comparison.No-thrash verification: legitimate multi-size use (icons rendered at several fixed sizes at once, theme picker previews at different form factors) is unaffected as long as the number of concurrently-displayed sizes for one asset stays at or under 8 — every steady-state frame is a cache hit, so the LRU pass never observes a miss and never evicts. This is covered by
test_image_does_not_evict_active_size_variants_at_capacity, which repeatedly re-requests the same 8 sizes across simulated paint frames and asserts every result isRc::ptr_eqto the original (i.e., served from cache, not recomputed).Linked Issue
specs/APP-3877/TECH.md(Prep 1; this PR implements the "main changeset" it defers to)Changes
crates/warpui_core/src/image_cache.rs: addedRenderedCacheEntry(wraps the cachedRc<Image>with alast_accessed: Cell<u64>sequence number) andMAX_CACHED_SIZES_PER_ASSET; addedImageCache::next_access_sequence(); wired a lazy LRU eviction pass intoImageCache::image()that callsevict_size()when an asset's cache is at capacity; removed#[allow(dead_code)]and theTODO(APP-3877)fromevict_size().crates/warpui_core/src/image_cache_tests.rs: addedtest_image_evicts_lru_size_variant_when_cache_exceeds_capacity(drives real eviction throughimage()across more distinct sizes than the cap, and asserts evicted variants'Arc<StaticImage>strong counts reach zero) andtest_image_does_not_evict_active_size_variants_at_capacity(asserts no thrash for a steady-state multi-size workload, and proves that cache hits refresh LRU recency by re-hitting the oldest entry, forcing an eviction with a new size, and asserting the refreshed entry survives while the true next-oldest entry is evicted — this fails if the hit-path recency bump is removed).Testing
./script/format— no changes.cargo clippy -p warpui_core --all-targets --all-features --tests -- -D warnings— clean (scoped to the crate touched; this change does not modify any public API surface ofwarpui_coreconsumed elsewhere, so no other crate's clippy pass is affected).cargo nextest run -p warpui_core— 315 tests run, 315 passed, 7 skipped (including the two new tests and the existingevict_size/evict_imagedirect-call tests, which continue to pass unmodified).No UI/visual changes; this is a pure memory-management fix in
ImageCache's internals, so no computer-use verification was performed.I have manually tested my changes locally with
./script/run— not run in this sandboxed environment; verification relied on the unit tests above, which exercise the exact code path (ImageCache::image()) that the affected UI calls every frame.Screenshots / Videos
Not applicable — no user-visible or UI changes.
Agent Mode
CHANGELOG-BUG-FIX: Fixed a memory leak where Warp's image cache would grow without bound when the same image was repeatedly rendered at different sizes (for example, while resizing a window with an image visible).
Co-Authored-By: Warp agent@warp.dev