From 38b1e0a8e7aafa1511bf09bd6061bbc07389552c Mon Sep 17 00:00:00 2001 From: Thanos Apollo Date: Mon, 5 Oct 2026 12:43:16 +0300 Subject: [PATCH] fix(textprop): reuse detached interval slots 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. --- .../buffer_text/tests/interval_retention.rs | 29 ++++ .../src/buffer/buffer_text/tests/mod.rs | 1 + crates/neovm-core/src/buffer/text_props.rs | 77 +++++++-- .../src/buffer/text_props/tests/mod.rs | 3 + .../src/buffer/text_props/tests/retention.rs | 147 ++++++++++++++++++ 5 files changed, 246 insertions(+), 11 deletions(-) create mode 100644 crates/neovm-core/src/buffer/buffer_text/tests/interval_retention.rs create mode 100644 crates/neovm-core/src/buffer/text_props/tests/retention.rs diff --git a/crates/neovm-core/src/buffer/buffer_text/tests/interval_retention.rs b/crates/neovm-core/src/buffer/buffer_text/tests/interval_retention.rs new file mode 100644 index 0000000000..11c945a7dc --- /dev/null +++ b/crates/neovm-core/src/buffer/buffer_text/tests/interval_retention.rs @@ -0,0 +1,29 @@ +use crate::emacs_core::eval::Context; + +#[test] +fn lisp_property_churn_reuses_interval_slots() { + crate::test_utils::init_test_tracing(); + let mut eval = Context::new(); + eval.eval_str( + "(progn (set-buffer (get-buffer-create \" interval-retention\")) + (setq buffer-undo-list t) + (insert (make-string 32 ?x)) + (let ((i 0)) + (while (< i 2000) + (set-text-properties 9 25 '(retention-probe 1)) + (set-text-properties 1 33 nil) + (setq i (1+ i)))))", + ) + .unwrap(); + let buffer = eval.buffers.current_buffer().unwrap(); + let (slots, capacity) = buffer + .text + .storage + .borrow() + .text_props + .arena_slot_counts_for_test(); + assert!( + slots <= 3 && capacity <= 4, + "set-text-properties churn retained {slots} slots, capacity {capacity}" + ); +} diff --git a/crates/neovm-core/src/buffer/buffer_text/tests/mod.rs b/crates/neovm-core/src/buffer/buffer_text/tests/mod.rs index 6fd0920a37..120dd12bde 100644 --- a/crates/neovm-core/src/buffer/buffer_text/tests/mod.rs +++ b/crates/neovm-core/src/buffer/buffer_text/tests/mod.rs @@ -9,6 +9,7 @@ use crate::emacs_core::value::Value; use super::BufferText; +mod interval_retention; mod reverse_scan_test; fn implemented_kind(kind: BufferTextBackendKind) -> ImplementedBufferTextBackendKind { diff --git a/crates/neovm-core/src/buffer/text_props.rs b/crates/neovm-core/src/buffer/text_props.rs index f566c7ef8c..36ae2ba15a 100644 --- a/crates/neovm-core/src/buffer/text_props.rs +++ b/crates/neovm-core/src/buffer/text_props.rs @@ -644,6 +644,10 @@ impl IntervalRun { struct IntervalTree { root: Option, nodes: Vec, + /// Head of the detached slots, chained through their `right` links. + /// `push_node` reuses them before growing `nodes`, so the arena stays + /// bounded by the peak number of simultaneously linked intervals. + free_head: Option, /// Positional last-descent memo for `find_id`: `(start, end, id)` of the most /// recently located interval, tagged with the tree version it was valid for. /// A lookup that lands inside `[start, end)` returns in O(1) instead of @@ -719,6 +723,7 @@ impl Clone for IntervalTree { Self { root: self.root, nodes: self.nodes.clone(), + free_head: self.free_head, version: AtomicU64::new(0), cache_gen: MemoGeneration::default(), cache_start: AtomicUsize::new(0), @@ -970,11 +975,38 @@ impl IntervalTree { fn push_node(&mut self, node: IntervalNode) -> IntervalId { self.invalidate_find_cache(); + if let Some(id) = self.free_head { + self.free_head = self.nodes[id.0].right; + self.nodes[id.0] = node; + return id; + } let id = IntervalId(self.nodes.len()); self.nodes.push(node); id } + /// Return `id` to the free list. The caller must already have removed + /// every link to it and must not use `id` afterwards; live ids never move. + fn recycle_node(&mut self, id: IntervalId) { + self.invalidate_find_cache(); + let node = &mut self.nodes[id.0]; + node.left = None; + node.parent = None; + node.total_length = CharLen::ZERO; + node.plist = Value::NIL; + node.refresh_cache(); + node.right = self.free_head; + self.free_head = Some(id); + } + + /// Drop every interval. `nodes` keeps its capacity for reuse. + fn clear(&mut self) { + self.invalidate_find_cache(); + self.root = None; + self.nodes.clear(); + self.free_head = None; + } + fn leftmost_id(&self, mut id: IntervalId) -> IntervalId { while let Some(left) = self.nodes[id.0].left { id = left; @@ -1078,8 +1110,8 @@ impl IntervalTree { /// Node `id`, reached through `root` or another node's link, without /// the bounds check. Every id a tree holds was minted by `push_node` as - /// the index it pushed at, and `nodes` only grows -- `delete_node` - /// unlinks a node and leaves its slot in place, and a rebuilt tree comes + /// an allocated slot; `recycle_node` reuses only unlinked slots, `clear` + /// truncates `nodes` only together with the root, and a rebuilt tree comes /// with its own `nodes` -- so a linked id is always in bounds. The /// balancing code below is the one user: it reads and rewrites a handful /// of neighbouring nodes per step, and the checks were over a quarter of @@ -1720,6 +1752,29 @@ impl IntervalTree { if let Some(root) = self.root { walk(self, root, None); } + // Every arena slot is either linked into the tree or on the free list. + fn linked_ids(tree: &IntervalTree, id: Option, seen: &mut FxHashSet) { + if let Some(id) = id { + assert!(seen.insert(id.0), "interval {} linked twice", id.0); + linked_ids(tree, tree.nodes[id.0].left, seen); + linked_ids(tree, tree.nodes[id.0].right, seen); + } + } + let mut seen = FxHashSet::default(); + linked_ids(self, self.root, &mut seen); + let mut free = self.free_head; + while let Some(id) = free { + assert!( + seen.insert(id.0), + "free interval {} is linked or cyclic", + id.0 + ); + let node = &self.nodes[id.0]; + assert!(node.left.is_none() && node.parent.is_none()); + assert!(node.total_length.is_empty() && node.plist.is_nil()); + free = node.right; + } + assert_eq!(seen.len(), self.nodes.len(), "unaccounted interval slots"); } #[cfg(test)] @@ -1963,13 +2018,7 @@ impl IntervalTree { } } - let node = &mut self.nodes[id.0]; - node.left = None; - node.right = None; - node.parent = None; - node.total_length = CharLen::ZERO; - node.plist = Value::NIL; - node.refresh_cache(); + self.recycle_node(id); } fn interval_deletion_adjustment( @@ -2029,7 +2078,7 @@ impl IntervalTree { let mut left_to_delete = end.min(tree_len).saturating_sub(start); if left_to_delete == tree_len { - self.root = None; + self.clear(); return; } @@ -2038,7 +2087,7 @@ impl IntervalTree { return; }; if left_to_delete == self.nodes[root.0].total_length { - self.root = None; + self.clear(); return; } let deleted = self.interval_deletion_adjustment(root, start, left_to_delete); @@ -2110,6 +2159,7 @@ impl IntervalTree { self.invalidate_find_cache(); } + self.recycle_node(id); Some(removed_len) } @@ -4858,6 +4908,11 @@ impl TextPropertyTable { self.intervals.max_depth_for_test() } + #[cfg(test)] + pub(crate) fn arena_slot_counts_for_test(&self) -> (usize, usize) { + (self.intervals.nodes.len(), self.intervals.nodes.capacity()) + } + /// `(start, end, plist pairs)` of every interval, empty ones included. #[cfg(test)] pub(crate) fn interval_plist_runs_for_test(&self) -> Vec { diff --git a/crates/neovm-core/src/buffer/text_props/tests/mod.rs b/crates/neovm-core/src/buffer/text_props/tests/mod.rs index 966bf24163..77fd5f6b0e 100644 --- a/crates/neovm-core/src/buffer/text_props/tests/mod.rs +++ b/crates/neovm-core/src/buffer/text_props/tests/mod.rs @@ -1,5 +1,8 @@ use super::*; +#[cfg(test)] +mod retention; + #[cfg(test)] mod source_slice_graft; diff --git a/crates/neovm-core/src/buffer/text_props/tests/retention.rs b/crates/neovm-core/src/buffer/text_props/tests/retention.rs new file mode 100644 index 0000000000..5421231a1d --- /dev/null +++ b/crates/neovm-core/src/buffer/text_props/tests/retention.rs @@ -0,0 +1,147 @@ +use super::*; + +/// Set a property on the middle of a fixed 32-character object, then clear +/// the whole object: every cycle splits and merges intervals without changing +/// how many are needed at once. +fn churn(table: &mut TextPropertyTable, cycles: usize) { + let name = Value::symbol("retention-probe"); + for _ in 0..cycles { + set_chars_for_object_len(table, 8, 24, 32, vec![(name, Value::fixnum(1))]); + set_chars_for_object_len(table, 0, 32, 32, Vec::new()); + } +} + +#[test] +fn fixed_size_property_churn_reuses_detached_slots() { + crate::test_utils::init_test_tracing(); + let mut table = TextPropertyTable::new(); + churn(&mut table, 20_000); + assert_eq!(table.intervals.runs().len(), 1); + let slots = table.intervals.nodes.len(); + let capacity = table.intervals.nodes.capacity(); + assert!(slots <= 3, "fixed-size churn retained {slots} slots"); + assert!( + capacity <= 4, + "fixed-size churn retained capacity {capacity}" + ); + table.intervals.assert_invariants_for_test(); +} + +#[test] +fn recycled_slots_keep_cow_snapshots_independent() { + use std::rc::Rc; + crate::test_utils::init_test_tracing(); + let name = Value::symbol("retention-probe"); + let mut current = Rc::new(TextPropertyTable::new()); + churn(Rc::make_mut(&mut current), 100); + // Both sides of the copy-on-write split start with the same free slots + // and must reuse them without disturbing each other. + let mut other = Rc::clone(¤t); + Rc::make_mut(&mut current).set_properties_for_object_char_len( + char_range(4, 28), + char_len(32), + vec![(name, Value::fixnum(7))], + ); + let frozen = current.clone(); + let frozen_runs = frozen.interval_plist_runs_for_test(); + churn(Rc::make_mut(&mut other), 2_000); + churn(Rc::make_mut(&mut current), 2_000); + assert_eq!(frozen.interval_plist_runs_for_test(), frozen_runs); + assert_eq!(get_at_char(&frozen, 16, name), Some(Value::fixnum(7))); + assert_eq!(get_at_char(&other, 16, name), None); + assert_eq!(get_at_char(¤t, 16, name), None); + for table in [¤t, &other, &frozen] { + table.intervals.assert_invariants_for_test(); + assert!(table.intervals.nodes.len() <= 5); + } +} + +#[test] +fn trailing_prune_and_whole_delete_reuse_retired_slots() { + crate::test_utils::init_test_tracing(); + let name = Value::symbol("retention-probe"); + let mut table = TextPropertyTable::new(); + for _ in 0..5_000 { + set_chars(&mut table, 8, 24, vec![(name, Value::fixnum(1))]); + // Seed the position and rightmost memos right before retirement. + assert!(table.intervals.find_id(char_pos(16)).is_some()); + clear_chars(&mut table, 0, 32); + assert!(table.is_empty()); + table.intervals.assert_invariants_for_test(); + assert!(table.intervals.nodes.len() <= 2); + set_chars_for_object_len(&mut table, 4, 28, 32, vec![(name, Value::fixnum(2))]); + delete_char_range(&mut table, 0, 32); + assert_eq!(table.intervals.nodes.len(), 0); + assert!(table.intervals.find_id(char_pos(0)).is_none()); + table.intervals.assert_invariants_for_test(); + } + assert!(table.intervals.nodes.capacity() <= 4); +} + +#[test] +fn recycling_matches_character_model_under_mixed_edits() { + crate::test_utils::init_test_tracing(); + let name = Value::symbol("syntax-table"); + let mut table = TextPropertyTable::new(); + let mut expected = vec![None; 32]; + let mut seed = 0x81a3_769du64; + let mut random = || { + seed ^= seed << 13; + seed ^= seed >> 7; + seed ^= seed << 17; + seed as usize + }; + let mut peak = 0; + for step in 0..5_000 { + let start = random() % expected.len(); + let end = start + 1 + random() % (expected.len() - start); + let value = Value::fixnum((step % 7 + 1) as i64); + match random() % 6 { + 0 => { + set_chars_for_object_len( + &mut table, + start, + end, + expected.len(), + vec![(name, value)], + ); + expected[start..end].fill(Some(value)); + } + 1 => { + put_chars_for_object_len(&mut table, start, end, expected.len(), name, value); + expected[start..end].fill(Some(value)); + } + 2 => { + remove_chars(&mut table, start, end, name); + expected[start..end].fill(None); + } + 3 if expected.len() < 64 => { + insert_chars_at(&mut table, start, 1); + expected.insert(start, None); + } + 4 if expected.len() > 8 => { + delete_char_range(&mut table, start, start + 1); + expected.remove(start); + } + _ => { + clear_chars(&mut table, start, end); + expected[start..end].fill(None); + } + } + table.intervals.assert_invariants_for_test(); + assert!(table.debug_syntax_caches_consistent().is_ok()); + peak = peak.max(table.intervals.runs().len()); + assert!(table.intervals.nodes.len() <= peak + 2); + for (pos, value) in expected.iter().enumerate() { + assert_eq!( + get_at_char(&table, pos, name), + *value, + "step {step}, pos {pos}" + ); + assert_eq!( + table.intervals.find_id(char_pos(pos)), + table.intervals.find_id_uncached(char_pos(pos)) + ); + } + } +}