sum_tree: fix SumTree::push to merge into existing leaves (APP-5507 memory fix) - #15278
Draft
warp-agent-staging[bot] wants to merge 2 commits into
Draft
sum_tree: fix SumTree::push to merge into existing leaves (APP-5507 memory fix)#15278warp-agent-staging[bot] wants to merge 2 commits into
warp-agent-staging[bot] wants to merge 2 commits into
Conversation
Buffer::edit builds the full new content of a notebook/markdown buffer by calling SumTree::push once per style/block marker and newline while constructing the replacement content tree. SumTree::push wrapped every pushed item in its own single-item leaf plus a throwaway Internal node (via from_child_trees) before merging it in via push_tree. Once the tree grew past its first leaf, that extra Internal wrapper made push_tree_recursive treat the item as a same-height sibling subtree, so it appended a brand new one-item leaf instead of topping up the last leaf that still had room. Leaves are fixed-capacity ArrayVecs (2 * TREE_BASE slots), so a one-item leaf permanently allocates the same size as a full one. Every push() past the first few items therefore retained a mostly-empty leaf node for the lifetime of the tree, which is exactly what the Sentry heap profile for APP-5507 observed as ~9GB of retained inuse_space under SumTree::push/push_tree_recursive during FileNotebookView::open_local. Fix push() to hand push_tree a plain (unwrapped) single-item leaf, the same shape extend() already uses for its trailing partial leaf. This takes the height-delta descent path that merges into the last under-full leaf when there is room, instead of the flat same-height append path. Fixes APP-5507.
Contributor
Author
|
This PR was generated with Warp. Comment |
Remove the history-narrative comments flagged in review: state what the current code does (an unwrapped single-item leaf is always underflowing, so push_tree descends and tops up the last leaf) instead of describing what the old implementation used to do.
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
Fixes APP-5507: opening a local notebook file can retain ~9GB of live heap building the buffer's content
SumTree<BufferText>(Sentry 7259255054).Confirmed mechanism
Buffer::edit(the pathFileNotebookView::open_local-> ... ->Buffer::from_markdown->replace_with_formatted_text->apply_core_edit_action->Buffer::editruns through for a full-buffer load) builds the entire new content tree from scratch, appending most text viaSumTree::append_str(already batched throughSumTree::extend) but appending every style marker, block marker, link/color marker and newline one at a time viaSumTree::push.SumTree::pushwrapped each pushed item in its own single-item leaf and a throwawayInternalnode (SumTree::from_child_trees(vec![leaf])) before merging it in viapush_tree. Leaves are fixed-capacityArrayVecs (2 * TREE_BASEslots, 12 in production), so this extra wrapping isn't just churn: once the tree grows past its first leaf, the wrapper makespush_tree_recursivetreat the new item as a same-height sibling subtree and append it as a brand-new one-item leaf, instead of taking the height-delta descent path that tops up the last leaf with room. Everypush()past the first dozen or so items therefore permanently retains a mostly-empty leaf for the lifetime of the buffer/tree - this is genuinely retained memory, not transient churn, which matches the ticket'sinuse_space(notalloc_space) profile.I verified this by reproducing the tree-shape defect directly in
sum_tree(seetest_repeated_push_packs_leaves_as_densely_as_extend, which failed before the fix: pushing N items one at a time produced a much taller/wider tree than building the same N items viaextend), and by measuring retained bytes with a small allocator-counting harness (not part of this PR): pushing 200k items one at a time retained 96 bytes/item before the fix vs. 32 bytes/item after, with productionTREE_BASE. I could not reproduce the exact ~9GB/notebook scenario in this environment (no jemalloc heap-profiling harness or a notebook of that size available here), so that reduction is illustrative of the structural fix, not a direct restatement of the Sentry numbers.A second occurrence corroborates the allocation site: Sentry event 6e1fb0cffa724a889cb501c0602023c9 (same issue, ~8.47GB sampled
inuse_space, macOS stable v0.2026.07.01.09.21 on an Apple M3 Max) puts ~56% (~4.7GB) of sampled bytes on the sameArc<Node<BufferText>>::newsite underSumTree::push. Two events on different machines and different builds agreeing on the dominant site is the reason this fix targetspushrather than the wider call chain. That second event is from an older build than the first, so it is a second sighting of the same bug rather than evidence about the fix.I did not find evidence for the other two candidate mechanisms called out in the ticket:
update_content_with_text_fragments's per-fragmentcontent_tree.extent::<StyleSummary>()call is O(1) (SumTree::extentjust reads the cached root summary), so that loop is not quadratic.Buffer::edit'sold_content.clone()only Arc-clones the tree root to build a read cursor over the old content; the new content is grown independently via cursor slices + pushes, so there's no COW amplification from a held Arc reference on this path specifically.Change
crates/sum_tree/src/lib.rs:SumTree::pushnow pushes a plain, unwrapped single-item leaf - the same shapeSumTree::extendalready uses for its trailing partial leaf. An unwrapped single-item leaf is always underflowing, sopush_treedescends into and tops up the last leaf that has room, instead of appending a singleton sibling.push's external contract (item order, summaries, cursor behavior) is unchanged, and the same descent path is already exercised byextend's existing behavior.Blast radius
sum_tree::SumTree::pushis used throughoutcrates/editor(buffer/notebook content construction, ~40+ call sites) and bycrates/warp_tuiandcrates/warpui_core(list/table view models). The change is additive/behavior-preserving at the API level, and should reduce memory retention for every caller that builds a tree via repeated single-item pushes, not just the notebook path. I ran the fullsum_treeandwarp_editortest suites andcargo checkforwarp_tui/warpui_coreto confirm nothing else broke.Linked Issue
Linear: APP-5507 (no corresponding GitHub issue).
Testing
cargo nextest run -p sum_tree: all tests pass, including two new regression tests:test_push_matches_extend_for_same_items: pushing items one at a time produces the same item sequence asextend.test_repeated_push_packs_leaves_as_densely_as_extend: pushing items one at a time now produces a tree of the same height asextend(this test fails against the pre-fix implementation).cargo nextest run -p warp_editor: all 484 tests pass (buffer/notebook content construction is exercised extensively here).cargo fmt -p sum_tree -- --checkandcargo clippy -p sum_tree --all-targets --all-features --tests -- -D warnings: clean.cargo clippy -p warp_editor --all-targets --all-features --tests -- -D warnings: clean.cargo check -p warp_tui -p warpui_core: compiles cleanly against the changedsum_treeAPI../script/runsession was used; this is not a UI-visible change).Agent Mode