Repository navigation
fix(textprop): reuse detached interval slots - #479
thanosapollo wants to merge 1 commit into
Conversation
The interval arena only ever grew: merging, deleting or pruning an interval unlinked its node but left the slot behind, and push_node always appended. Repeatedly re-propertizing the same text therefore grew the arena without bound while the number of live intervals stayed constant -- 2,000 set-text-properties/clear cycles over a 32-character buffer left 4,001 slots for one live interval. Chain unlinked slots into an intrusive free list through their unused right link and let push_node take from it before growing. Both retirement points feed it: delete_zero_length_interval and the trailing-prune remove_rightmost_interval, which previously left its node and plist in place. A whole-range delete_range now clears the arena, keeping its capacity. Recycling and clearing bump the tree version, so no memo can return a reused id, and live ids never move. A copy-on-write clone copies the free list with the arena. The arena is now bounded by the peak number of simultaneous intervals. The test invariant check also verifies that every slot is linked exactly once or free.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (5)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughThe interval tree now recycles detached node slots instead of continually growing its arena during text-property edits. New tests check slot and capacity bounds, copy-on-write independence, deletion behavior, and property lookups after mixed edits. ChangesText property interval retention
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to No concrete issue currently blocks merging the interval-slot reuse change. Security Architecture ReviewSecurity architecture risk: ⚪ Minimal · up to The inspected change introduces no material security risk. Retired slots are cleared before reuse, cached lookups are invalidated, and cloned snapshots retain independent storage. Existing text-property entrypoints and their controls are unchanged. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
thanos already carries the content of these heads, verbatim or as the personal variant it ships; this merge keeps the tree and records the heads so later upstream merges of them need no re-resolution. - contrib/repeat-backpressure 551220e (eval-exec#459): carried; the rebase only moved to FrontendKey, as the main merge did. - contrib/nested-reader-recovery 91a4022 (eval-exec#460): carried, combined with the command-error reporting below. - fix/command-error-literal-message 271a202 (eval-exec#489): carried as 56339fa, 98b7742, 0ebd026. - fix/non-ascii-face-width 9a52a76 (eval-exec#481): carried as 13ad539 and 7eee365. - feature/builtin-mcp c9542d3 (eval-exec#480): personal endpoint variant with the same reply bound (677591e) and documentation (e0ee6c3). - fix/text-prop-interval-recycling 38b1e0a (eval-exec#479): personal interval recycling passes the same retention tests. - feature/gui-daemon-publication 1cd96f8 (eval-exec#454): personal deferred GUI daemon, a superset. Kept deliberately: terminal-live-p classifies every terminal by its output method (GNU Fterminal_live_p), an unregistered frame's native-window wait fails closed, and neomacs-set-frame-opacity passes integer percentages through unchanged, since alpha-background already reads a fixnum as a percentage (GNU gui_set_alpha_background).
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
It modifies a core, concurrency-sensitive interval arena with subtle free-list/memo-invalidation and copy-on-write invariants whose correctness warrants human verification despite comprehensive tests.
Review effort: Balanced
Findings: None
What changed in this PR
This PR fixes unbounded memory growth in the text-property interval arena (IntervalTree::nodes). Previously, when intervals were merged, deleted, or pruned, their arena slots were unlinked but never reused — push_node always appended — so churn-heavy workloads (e.g. repeated set-text-properties followed by clearing) grew the arena indefinitely even though the live interval count stayed constant. The fix introduces an intrusive free list so retired slots are reused, bounding the arena by the peak simultaneous interval count.
Changes:
- Added a
free_headfree list chained through each node's now-unusedrightlink;push_nodereuses a free slot before growingnodes, and both retirement points (delete_zero_length_interval,remove_rightmost_interval) recycle via a newrecycle_node. - A whole-range
delete_rangenow calls a newclear()that truncatesnodes(keeping capacity); recycling and clearing bumpversionso position/finger/rightmost memos cannot return a reused id, and COW clones copy the arena with its free list. assert_invariants_for_testnow verifies every slot is linked exactly once or on the free list, plus new regression tests at the table and Lisp level.
| File | Description |
|---|---|
crates/neovm-core/src/buffer/text_props.rs |
Core change: free_head, push_node reuse, recycle_node, clear, updated invariants/docs, COW clone, and test accessor. |
crates/neovm-core/src/buffer/text_props/tests/retention.rs |
New unit tests for slot reuse, COW independence, prune/whole-delete, and a randomized differential model. |
crates/neovm-core/src/buffer/text_props/tests/mod.rs |
Registers the new retention test module. |
crates/neovm-core/src/buffer/buffer_text/tests/interval_retention.rs |
New Lisp-level set-text-properties churn regression test. |
crates/neovm-core/src/buffer/buffer_text/tests/mod.rs |
Registers the new interval_retention test module. |
I verified the free-list invariants (slots recycled only after full unlinking, no double-free given the cycle check), memo correctness (all three mutation sites bump version, invalidating version-tagged memos), COW clone semantics, build_balanced/from_normalized_runs only running on empty arenas, and that no full-arena traversal ever reads free slots. I found no objective defects to comment on.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
eval-exec
added
the
merge-conflict
Text-property intervals live in an index arena (
IntervalTree::nodes). When an interval is merged, deleted or pruned, its node is unlinked but the slot stays in the arena, andpush_nodealways appends. So any workload that keeps splitting and re-merging the same text grows the arena for as long as the table lives, even though the number of live intervals stays constant. Examples are aset-text-propertiescall followed by clearing the same range, or deleting text that spans interval boundaries. Each node is about 80 bytes on 64-bit targets.Reproducer on current
main: in a 32-character buffer with undo disabled, 2,000 iterations of(set-text-properties 9 25 '(p 1))followed by(set-text-properties 1 33 nil)leave 4,001 arena slots (capacity 4,096) for one live interval. Doing the same thing directly on the table 20,000 times leaves 40,001 slots.Change
rightlink.push_nodetakes a slot from that list before it growsnodes. Live ids never move.delete_zero_length_interval(merge/delete), which already reset the node, andremove_rightmost_interval(trailing prune), which used to leave its node and plist behind.delete_rangenow clears the arena (keeping its capacity) instead of only dropping the root.version, so the position, finger and rightmost memos cannot return a reused id. A copy-on-write clone copies the arena together with its free list, and each side then recycles on its own.assert_invariants_for_testnow checks that every slot is either linked exactly once or on the free list, and that free slots are nil and zero-length.The arena's size is now bounded by the peak number of intervals that exist at the same time. Capacity reached in the past is not shrunk.
Tests
New regressions are in
buffer/text_props/tests/retention.rsandbuffer/buffer_text/tests/interval_retention.rs:set-text-propertieschurn above stays at ≤3 slots/≤4 capacityAll five fail on
7764ef71d2(for example,fixed-size churn retained 40001 slots,set-text-properties churn retained 4001 slots, capacity 4096) and pass with this change.With Nextest 0.9.146 (
neovm-core --lib, 2 test threads):test(/interval_retention::|text_props::|textprop|text_prop|buffer_text::|indirect|interval/).test(/^buffer::|pdump|syntax|undo|fontif|font_lock|gc_|textprop|text_prop|propert/). The 8 failures fail identically on unmodified7764ef71d2in the same checkout: 2chartabletests can't find the generated Unicode tables, and 6doc/load/vmtests fail loadingemacs-lisp/pcasefrom source.cargo clippy -p neovm-core --lib --testsreports nothing in the changed files.No full-workspace or GUI run is claimed.
Found while tracking down memory growth in a long-running session: repeated text-property updates kept growing the interval arena.
This PR is agent-assisted.