Expire stale positions in the WarpUI PositionCache (APP-5348) - #15055
Draft
warp-agent-staging[bot] wants to merge 2 commits into
Draft
Expire stale positions in the WarpUI PositionCache (APP-5348)#15055warp-agent-staging[bot] wants to merge 2 commits into
warp-agent-staging[bot] wants to merge 2 commits into
Conversation
`PositionCache::committed_positions` was only ever inserted into. `SavePosition::paint` re-caches a position every frame via `cache_position_indefinitely`, but nothing removed the entry once the element stopped painting, so the map retained one entry per UI entity ever painted for the lifetime of the window. `AppContext::build_scene` also deep-clones the whole cache into `last_frame_position_cache` every frame, so the growth became per-frame allocation churn as well. Committed positions now carry the frame they were last cached in, and the per-frame reset expires any that have not been re-cached for 600 frames. Consumers read positions during the layout pass that precedes the next paint, so the entry only has to outlive its last paint by one frame; 600 leaves a wide margin for transiently unpainted elements. Co-Authored-By: Warp Agent <agent@warp.dev>
Contributor
Author
|
This PR was generated with Warp. |
The expiry counter advances once per `Presenter::build_scene`, but `AppContext::build_scene` loops `for iter in 1..=3` and can run the invalidate/layout/paint pass up to three times per presented redraw while synthesizing hover events (app.rs:2918,2947). The constant, the field, and the counter all said "frame", so a 600-frame lifetime was really as few as 200 redraws in the worst case. Rename them to say what they count, and size the constant against the worst case so the intended margin holds: 1800 scene builds guarantees at least 600 presented redraws. The longer TTL costs nothing meaningful here since the whole leak is kilobytes to a few megabytes. `test_committed_positions_survive_the_worst_case_redraw_budget` drives the cache through three-build redraws and asserts the redraw margin explicitly, so lowering the constant fails with a message naming the shortfall rather than quietly shrinking the window. Also drop the `CommittedPosition` doc comment, which restated the type name and its fields instead of explaining anything. Co-Authored-By: Warp Agent <agent@warp.dev>
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.


Description
Important
This does not close APP-5348. The ticket's stated hypothesis — that the
Container/Flex/Stackpaint/layout pipeline retains memory across re-renders — is not supported by the code. This PR fixes a real but small monotonic leak I found in that pipeline while investigating. The driver of the reported 9.51 GB footprint is conversation retention, tracked in APP-5231.APP-5348 was filed by the Sentry memory triage bot off a symbolicated heap profile (9.51 GB sampled
inuse_space) that attributed 28.39% toContainer::Element::paint, 25.86% toFlex::Element::paint, 22.21% to_warp_update_layer, and 16.92% tobuild_scene. It asked whetherContainer/Flex/Stackpaint/layoutare missing caching/diffing, whether the scene graph grows unbounded, or whether relayed task outputs retain closures/Arcs tied to code-diff views.Why the ticket's hypothesis does not hold
Those percentages are cumulative-by-function aggregates over a recursive element tree, so they double-count:
Container28.39% andFlex25.86% both exceedbuild_scene's own 16.92%, which is impossible for disjoint frames in a call tree. They mark the allocation site of a legitimately-live scene, not a leak. Reading the code confirms it:Scene::newinPresenter::paint,crates/warpui_core/src/presenter.rs:423) and the previous one is dropped when replaced (presenter.rs:375-376).WindowState::next_sceneis anOptionoverwritten per frame (crates/warpui/src/platform/mac/window.rs:1412-1428,:1196). Nothing appends across frames.Container::paintonly pushes a rect into the current scene (crates/warpui_core/src/elements/gui/container.rs:295-358);Flex::paintjust walks its children (crates/warpui_core/src/elements/gui/flex/mod.rs:402-501), and in release itslocation_infois a&'static str(flex/mod.rs:415-416). Neither allocates anything that outlives the pass.Presenter::rendered_viewsis keyed by view id, overwritten on re-render and removed when the view is dropped (presenter.rs:318-331,crates/warpui_core/src/core/app.rs:3432-3444).LayoutCacheis a strict two-frame swap-and-clear (crates/warpui_core/src/text_layout.rs:75-80).crates/warpui_core/src/core/autotracking/mod.rs:259-269), as doesview_parentsinremove_dropped_items(app.rs:3440).CodeDiffView::set_candidate_diffsreplacespending_diffsand carries adebug_assert!that it is only ever called once per view (app/src/ai/blocklist/inline_action/code_diff_view.rs:882-886,:931). Candidate diffs do not accumulate.RequestFileEditsExecutor's per-action state —diff_storagesanddiff_application_failures, which hold the prepared file contents — is already discarded at the terminal-result choke point (discard_pending,app/src/ai/blocklist/action_model/execute/request_file_edits.rs:130-133, called fromdiscard_action_state,execute.rs:930-938).What this PR actually fixes
One structure in the render pipeline only ever grows:
PositionCache::committed_positions(presenter.rs).SavePosition::paintre-caches a position on every paint pass throughcache_position_indefinitely(crates/warpui_core/src/elements/gui/stack/save_position.rs:60-66), but nothing removed the entry once the element stopped painting — a dropped view, a dismissed diff banner, a block scrolled out of the virtualized blocklist. Ids are unique per UI entity, so the map retained one entry for every element ever painted, for the window's lifetime:CodeDiffViewmints a randomposition_id_prefixper instance (code_diff_view.rs:406-407)context_menu_button_{block_index}per block (app/src/terminal/block_list_element.rs:1093)app/src/editor/view/element.rs:1065,:2051)Only one call site ever calls
clear_position(element.rs:1269). The leak is also amplified:AppContext::build_scenedeep-clones the entire cache — everyStringkey included — intolast_frame_position_cacheon every scene build (app.rs:2955-2956), so its size became per-build allocation churn as well.Committed positions now record the scene build they were last cached in, and the existing per-build reset (
clear_single_frame_positions) advances a counter and drops entries not re-cached forCOMMITTED_POSITION_SCENE_BUILD_LIFETIME(1800) scene builds. The public API is unchanged.Honest sizing: this is worth kilobytes to a few megabytes over a long session, plus the per-build clone cost. It is not 9.51 GB.
Why the clock counts scene builds, why 1800, and why no consumer breaks
The expiry clock ticks once per
Presenter::build_scene, not once per presented redraw.AppContext::build_sceneloopsfor iter in 1..=3and can run the invalidate/layout/paint pass up to three times per redraw while synthesizing hover events (crates/warpui_core/src/core/app.rs:2918,:2947). Budgeting the constant per redraw would therefore have silently given only a third of the intended margin in the worst case. The names and the doc comment say "scene build" so the code cannot drift back into claiming otherwise, and the constant is sized against the worst case: 1800 scene builds guarantees at least 600 presented redraws even when every redraw costs three builds. A test asserts that margin explicitly, so lowering the constant fails loudly rather than quietly shrinking the window.The longer TTL is essentially free — the entire reclaimed footprint is kilobytes to a few megabytes — so the margin is bought at no meaningful cost and is strictly safer for any anchor the audit below missed.
Every read goes through
PositionCache::get_position. The callers areStackpositioned children (crates/warpui_core/src/elements/gui/stack/offset_positioning.rs:496,:659,:702,:865),EventContext::element_position_by_id(presenter.rs:310),ViewContext::element_position_by_id(crates/warpui_core/src/core/view/context.rs:34),ClippedScrollable(clipped_scrollable.rs:328),Draggable(elements/gui/drag/draggable.rs:469-492),Slider, and thenew_scrollableaxis configs. In every case the anchor is in the same element tree and re-caches on each paint of that tree, so it never ages out while it is in use.The one hard ordering constraint:
Presenter::build_scenerunslayout()beforepaint()(presenter.rs:354-369), so layout reads positions committed by the previous build's paint. An entry must therefore outlive its last paint by at least one scene build. 1800 leaves a wide margin.A missing position for a conditionally-rendered anchor is already an explicitly supported outcome rather than a regression:
PositioningAnchor::conditionalexists precisely for anchors that are only sometimes rendered (offset_positioning.rs:405-429), andsize_constraintfalls back to the default constraint instead of panicking (offset_positioning.rs:111-133). For a non-conditional anchor a missing position already trips adebug_assert, and a non-conditional anchor paints on every scene build, so it never expires.The offscreen-blocklist case resolves cleanly too.
BlockListElementonly builds elements forvisible_items(block_list_element.rs:667-671), so a scrolled-away block stops re-cachingcontext_menu_button_{block_index}— but that id is only written while the block is hovered (block_list_element.rs:1078-1096) and only read while the overflow menu is anchored to that hovered block. A block that has not painted across 1800 consecutive scene builds is not the hovered block. That is exactly the memory this reclaims.Note the clock is scene builds, not wall time: an idle window expires nothing, and only elements that stop painting while the window keeps building scenes age out.
What actually drives the reported footprint (APP-5231)
BlocklistAIHistoryModel::conversations_by_id(app/src/ai/blocklist/history_model.rs:267) is only ever removed from on explicit user delete or logout (remove_conversation_from_memory,:2208;reset,:2803). Every other path inserts. That matches the two profile frames the ticket lists but does not theorise about —Task::decode7.33% plusread_agent_conversation_metadata7.36%, roughly 1.4 GB of live conversation payloads.The compounding chain, which is the part APP-5231 does not yet capture: each retained conversation keeps its blocks alive, which keeps every
CodeDiffViewalive, which keeps oneCodeEditorViewper edited file alive holding that file's full original content (FileDiff::new(diff.original_content, ...),request_file_edits.rs:315; editor reset atcode_diff_view.rs:898-902). Every frame then lays out and paints a scene sized by all of that retained content — which is precisely why the bytes land onContainer::paintandFlex::paintin the profile.Bounding that map is a design decision with its own correctness surface (live / cleared / active / parent-child / in-flight-rename references, and
has_local_datareloadability from SQLite), so it belongs in APP-5231 rather than riding along here.Linked Issue
APP-5348 — https://linear.app/warpdotdev/issue/APP-5348/memory-951-gb-warpui-renderpaint-pipeline-containerflexstack
Related: APP-5231 (agent conversations never evicted from
BlocklistAIHistoryModel.conversations_by_id)Sentry: https://sentry.io/organizations/warpdotdev/issues/7259255054/
ready-to-specorready-to-implement.Testing
Three unit tests in
crates/warpui_core/src/presenter_tests.rs:test_committed_positions_survive_a_brief_gap_in_painting— pins the contract that an entry outlives a gap in painting, guarding the layout-reads-previous-build's-paint ordering.test_committed_positions_expire_once_their_element_stops_painting— asserts the entry is gone at the lifetime boundary and thatcommitted_positionsitself shrank to a single entry, so the test proves the map stopped growing rather than just that the getter filters.test_committed_positions_survive_the_worst_case_redraw_budget— drives the cache through redraws that each cost the worst-case three scene builds, and pins both halves of the budget: the entry survives 600 such redraws, and it is gone (and removed from the map) after 1200. It also asserts the intended 600-redraw margin explicitly, so the constant cannot be lowered without a failure that names the shortfall.I confirmed both of the expiry tests genuinely fail against the bug they guard:
retaincall temporarily removed,test_committed_positions_expire_once_their_element_stops_paintingFAILED, then passed once restored.test_committed_positions_survive_the_worst_case_redraw_budgetFAILED withlifetime of 600 scene builds only guarantees 200 redraws, short of the intended 600, then passed once restored to 1800.Ran:
cargo nextest run -p warpui_core→ 316 passed, 0 failed.cargo nextest run -p warpui_core -p warpui→ 3 failures, all inwarpui windowing::winit::fonts::layout_tests::test_layout_text_first_line_indent_{small,medium,large}_bidirectional; I verified bygit stashthat they fail identically on unmodifiedmasterin this Linux container (missing RTL fonts), and they are unrelated to this change../script/format --check→ clean.cargo clippy -p warpui_core --all-targets --tests -- -D warnings→ clean. Also clean forwarpui,warpui_extras, andwarp_editor.What I could not verify. This was developed in a Linux container with a 3 GB memory ceiling, and this is the macOS GUI path.
The full presubmit clippy (
cargo clippy --workspace ...) could not complete:clippy-driveron thewarpapp crate is OOM-killed (signal: 9, SIGKILL). I confirmed viagit stashthat this reproduces identically on unmodifiedmasterhere, so it is the container limit and not this change.warp_tuiandintegrationclippy are blocked behind the same app-crate build. CI needs to be the gate for those.No
./script/run, so no manual or visual check that stack overlays still anchor correctly. The change is behaviour-preserving by construction for any element that keeps painting, and the argument for the elements that stop is written out above, but a reviewer with a Mac should sanity-check an overlay-heavy surface (the code-diff banner's scroll icon and accept menu, the blocklist overflow menu).I have manually tested my changes locally with
./script/run— not possible on Linux.Agent Mode