From b99405620eeeba0c6838a0d881d5b2082c2f0474 Mon Sep 17 00:00:00 2001 From: Raphael Amorim Date: Mon, 10 Aug 2026 10:47:30 +0200 Subject: [PATCH] rio-vt: intern extras by content, copy-on-write attach MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extras slots are now interned: identical (marks, hyperlink) content shares one slot, so the u16 id space stops being a per-occurrence budget whose overflow silently drops extras — repeated emoji and combining sequences cost one slot total. Shared slots make in-place mutation illegal, which exposes and fixes a live bug: every cell written under one OSC 8 template references the same slot, so pushing a combining mark into it attached the mark to the whole hyperlink span. The attach path is copy-on-write now: clone, extend, re-intern. Hyperlink span detection (hints, hover) compares the hyperlink itself instead of raw extras_id equality: a marked cell inside a link holds a different slot than its neighbors while belonging to the same span — under interning, and already before it, id equality was the wrong key. The sweep and free paths purge the interning lookup so stale content can't resolve to a dead slot. --- frontends/rioterm/src/hints.rs | 19 ++--- frontends/rioterm/src/screen/mod.rs | 14 ++-- rio-vt/src/crosswords/grid/mod.rs | 28 ++++++- rio-vt/src/crosswords/mod.rs | 112 +++++++++++++++++++++++----- rio-vt/src/crosswords/square.rs | 2 +- 5 files changed, 138 insertions(+), 37 deletions(-) diff --git a/frontends/rioterm/src/hints.rs b/frontends/rioterm/src/hints.rs index 6ab830179b..c429800273 100644 --- a/frontends/rioterm/src/hints.rs +++ b/frontends/rioterm/src/hints.rs @@ -254,11 +254,12 @@ impl HintState { ) { // Walk the visible region looking for OSC 8 hyperlink spans. // - // After the cell repack, hyperlinks live in the per-grid - // `extras_table`. Each cell carries an `extras_id: u16`; cells - // in the same hyperlink span share that id. We compare ids - // (cheap u16 compare) to find the start and end of each span, - // then look up the URI once via `Crosswords::cell_hyperlink`. + // Spans are found by comparing the hyperlink itself, not the + // cell's `extras_id`: extras slots are interned by content, so + // a cell that also carries combining marks holds a different + // slot than its neighbors while belonging to the same link. + // `Hyperlink` is an `Arc` around (id, uri); the comparison is + // content equality, matching the OSC 8 `id=` semantics. let grid = &term.grid; let display_offset = grid.display_offset(); let visible_lines = grid.screen_lines(); @@ -272,8 +273,8 @@ impl HintState { let mut col = 0usize; let cols = grid.columns(); while col < cols { - let id = match term.cell_hyperlink_id(line, Column(col)) { - Some(id) => id, + let link = match term.cell_hyperlink(line, Column(col)) { + Some(link) => link, None => { col += 1; continue; @@ -281,11 +282,11 @@ impl HintState { }; // Found the start of a hyperlink span. Walk forward - // until the extras_id changes. + // while the cells carry the same hyperlink. let start_col = col; let mut end_col = col; while end_col < cols - && term.cell_hyperlink_id(line, Column(end_col)) == Some(id) + && term.cell_hyperlink(line, Column(end_col)).as_ref() == Some(&link) { end_col += 1; } diff --git a/frontends/rioterm/src/screen/mod.rs b/frontends/rioterm/src/screen/mod.rs index f5b442d3f0..315f811c8c 100644 --- a/frontends/rioterm/src/screen/mod.rs +++ b/frontends/rioterm/src/screen/mod.rs @@ -2105,16 +2105,18 @@ impl Screen<'_> { // Look up the cell's hyperlink via the per-grid extras table. // Cells in the same OSC 8 span share an `extras_id`, so we - // walk left/right comparing ids (cheap u16 compare) to find - // the span boundaries, then look up the URI once. - let id = terminal.cell_hyperlink_id(point.row, point.col)?; + // walk left/right comparing the hyperlink itself (extras slots + // are interned by content, so a cell with combining marks has + // a different id while belonging to the same link) to find the + // span boundaries. + let hyperlink = terminal.cell_hyperlink(point.row, point.col)?; let mut start_col = point.col; let mut end_col = point.col; while start_col > rio_backend::crosswords::pos::Column(0) { let prev_col = start_col - 1; - if terminal.cell_hyperlink_id(point.row, prev_col) == Some(id) { + if terminal.cell_hyperlink(point.row, prev_col).as_ref() == Some(&hyperlink) { start_col = prev_col; } else { break; @@ -2122,15 +2124,13 @@ impl Screen<'_> { } while end_col < grid.columns() - 1 { let next_col = end_col + 1; - if terminal.cell_hyperlink_id(point.row, next_col) == Some(id) { + if terminal.cell_hyperlink(point.row, next_col).as_ref() == Some(&hyperlink) { end_col = next_col; } else { break; } } - let hyperlink = terminal.cell_hyperlink(point.row, point.col)?; - // Build a synthetic hint config so the rest of the hint // pipeline (highlighting, click action) treats this just like // a regex/url match. diff --git a/rio-vt/src/crosswords/grid/mod.rs b/rio-vt/src/crosswords/grid/mod.rs index 67bdd3a52d..b5defa23e8 100644 --- a/rio-vt/src/crosswords/grid/mod.rs +++ b/rio-vt/src/crosswords/grid/mod.rs @@ -126,6 +126,16 @@ impl ReflowRemap { pub struct ExtrasTable { slots: Vec>, free: Vec, + /// Content-hash interning: identical extras share one slot. + /// Emoji and combining sequences repeat constantly, and every + /// entry here used to burn a fresh slot toward the `u16::MAX` + /// cap (whose overflow silently drops extras). Invariant: every + /// live slot's content maps back to its id, maintained by + /// `alloc` and `sweep_unmarked`/`free`/`clear`. Interning also + /// means slots are shared — mutating one in place would edit + /// every referencing cell, so writers must copy-on-write + /// (clone, modify, re-alloc). + lookup: rustc_hash::FxHashMap, /// Allocations since the last mark-and-sweep. The table holds /// hyperlink and zero-width data whose slots stay referenced by /// cells until their rows scroll off the ring; without a periodic @@ -148,6 +158,7 @@ impl ExtrasTable { Self { slots: vec![None], free: Vec::new(), + lookup: rustc_hash::FxHashMap::default(), allocs_since_reclaim: 0, } } @@ -166,13 +177,19 @@ impl ExtrasTable { self.slots.get_mut(id as usize)?.as_mut() } - /// Allocate a new extras slot, returning its id (always non-zero). + /// Return the slot holding `extras`, interning by content: repeat + /// content reuses its existing slot, new content allocates one. + /// The returned id is always non-zero (0 on slot exhaustion). pub fn alloc( &mut self, extras: crate::crosswords::square::Extras, ) -> crate::crosswords::square::ExtrasId { + if let Some(&id) = self.lookup.get(&extras) { + return id; + } self.allocs_since_reclaim += 1; if let Some(id) = self.free.pop() { + self.lookup.insert(extras.clone(), id); self.slots[id as usize] = Some(extras); return id; } @@ -181,6 +198,7 @@ impl ExtrasTable { return 0; } let id = self.slots.len() as u16; + self.lookup.insert(extras.clone(), id); self.slots.push(Some(extras)); id } @@ -205,7 +223,9 @@ impl ExtrasTable { for id in 1..self.slots.len() { let marked = live[id / 64] & (1 << (id % 64)) != 0; if !marked && self.slots[id].is_some() { - self.slots[id] = None; + if let Some(extras) = self.slots[id].take() { + self.lookup.remove(&extras); + } self.free.push(id as u16); } } @@ -217,7 +237,8 @@ impl ExtrasTable { return; } if let Some(slot) = self.slots.get_mut(id as usize) { - if slot.take().is_some() { + if let Some(extras) = slot.take() { + self.lookup.remove(&extras); self.free.push(id); } } @@ -227,6 +248,7 @@ impl ExtrasTable { self.slots.clear(); self.slots.push(None); self.free.clear(); + self.lookup.clear(); } } diff --git a/rio-vt/src/crosswords/mod.rs b/rio-vt/src/crosswords/mod.rs index a15bb55c25..b45c14959b 100644 --- a/rio-vt/src/crosswords/mod.rs +++ b/rio-vt/src/crosswords/mod.rs @@ -3236,21 +3236,22 @@ impl Handler for Crosswords { column.0 = column.saturating_sub(1); } + // Copy-on-write: slots are interned and shared — every + // cell written under one OSC 8 template references the + // same slot, so pushing into it in place would attach the + // mark to the whole hyperlink span. Clone, extend, and + // re-intern instead; the marked cell gets its own + // (marks + hyperlink) slot and its neighbors keep theirs. + let existing_id = self.grid[row][column].extras_id(); + let mut extras = existing_id + .and_then(|id| self.grid.extras_table.get(id).cloned()) + .unwrap_or_default(); + extras.zerowidth.push(c); + let id = self.grid.alloc_extras(extras); let cell = &mut self.grid[row][column]; - let existing_id = cell.extras_id(); - if let Some(id) = existing_id { - if let Some(extras) = self.grid.extras_table.get_mut(id) { - extras.zerowidth.push(c); - } - } else { - let mut extras = crate::crosswords::square::Extras::default(); - extras.zerowidth.push(c); - let id = self.grid.alloc_extras(extras); - let cell = &mut self.grid[row][column]; - cell.set_extras_id(Some(id)); - cell.insert_cell_flag(CellFlags::GRAPHEME); - self.grid[row].has_extras = true; - } + cell.set_extras_id(Some(id)); + cell.insert_cell_flag(CellFlags::GRAPHEME); + self.grid[row].has_extras = true; return; } @@ -7404,6 +7405,78 @@ mod tests { assert!(!cw.grid.cursor.should_wrap); } + /// Identical extras content interns into one slot: the u16 id + /// space stops being a per-occurrence budget. + #[test] + fn extras_intern_by_content() { + use crate::performer::handler::Handler; + let mut cw = new_term(10, 2); + cw.input('e'); + cw.input('\u{301}'); + cw.input('e'); + cw.input('\u{301}'); + let a = cw.grid[Line(0)][Column(0)].extras_id().unwrap(); + let b = cw.grid[Line(0)][Column(1)].extras_id().unwrap(); + assert_eq!(a, b); + // Different content gets its own slot. + cw.input('e'); + cw.input('\u{302}'); + let c = cw.grid[Line(0)][Column(2)].extras_id().unwrap(); + assert_ne!(a, c); + } + + /// A combining mark typed inside an OSC 8 hyperlink must attach to + /// its own cell only. The old in-place mutation edited the shared + /// template slot, leaking the mark into every cell of the span — + /// and the marked cell must keep the hyperlink. + #[test] + fn combining_mark_inside_hyperlink_stays_on_one_cell() { + use crate::performer::handler::Handler; + let mut cw = new_term(10, 2); + let link = Hyperlink::new(None::<&str>, "https://example.com"); + cw.set_hyperlink(Some(link.clone())); + cw.input('a'); + cw.input('\u{301}'); + cw.input('b'); + cw.set_hyperlink(None); + + // The marked cell carries mark + hyperlink. + let a_id = cw.grid[Line(0)][Column(0)].extras_id().unwrap(); + let a = cw.grid.extras_table.get(a_id).unwrap(); + assert_eq!(a.zerowidth, vec!['\u{301}']); + assert_eq!(a.hyperlink.as_ref(), Some(&link)); + + // Its neighbor keeps the pristine hyperlink-only slot. + let b_id = cw.grid[Line(0)][Column(1)].extras_id().unwrap(); + let b = cw.grid.extras_table.get(b_id).unwrap(); + assert!(b.zerowidth.is_empty()); + assert_eq!(b.hyperlink.as_ref(), Some(&link)); + assert_ne!(a_id, b_id); + } + + /// The interning lookup must be purged when slots are swept, or a + /// re-alloc of old content would return a dead slot id. + #[test] + fn reclaim_purges_interning_lookup() { + use crate::performer::handler::Handler; + let mut cw = new_term(10, 2); + cw.input('e'); + cw.input('\u{301}'); + let content = cw + .grid + .extras_table + .get(cw.grid[Line(0)][Column(0)].extras_id().unwrap()) + .unwrap() + .clone(); + // Drop the only reference and sweep. + cw.clear_screen(ClearMode::All); + cw.grid.reclaim_extras(); + // Re-alloc of the same content must yield a live slot. + let id = cw.grid.alloc_extras(content.clone()); + assert_ne!(id, 0); + assert_eq!(cw.grid.extras_table.get(id), Some(&content)); + } + #[test] fn vs16_at_last_column_preserves_base_extras() { use crate::performer::handler::Handler; @@ -7421,10 +7494,15 @@ mod tests { cw.input('\u{FE0F}'); - // The wide base on the new row should still carry the same extras - // entry (the ZWJ we attached earlier). + // The wide base on the new row still carries the earlier ZWJ + // plus the VS16 that widened it. Slots are interned with + // copy-on-write now, so the id may change; the content is the + // contract (comparing ids would pin the old in-place-mutation + // behavior, which leaked pushes into shared slots). let moved_extras = cw.grid[Line(1)][Column(0)].extras_id(); - assert_eq!(moved_extras, original_extras); + assert!(moved_extras.is_some()); + let extras = cw.grid.extras_table.get(moved_extras.unwrap()).unwrap(); + assert_eq!(extras.zerowidth, vec!['\u{200D}', '\u{FE0F}']); assert_eq!(cw.grid[Line(1)][Column(0)].wide(), Wide::Wide); assert_eq!(cw.grid[Line(0)][Column(2)].wide(), Wide::LeadingSpacer); } diff --git a/rio-vt/src/crosswords/square.rs b/rio-vt/src/crosswords/square.rs index a500270d26..91e48e3bad 100644 --- a/rio-vt/src/crosswords/square.rs +++ b/rio-vt/src/crosswords/square.rs @@ -199,7 +199,7 @@ pub type ExtrasId = u16; /// Storage for the rare per-cell data that used to live inside `CellExtra`. /// Allocated only for cells that need it; pooled in a `Vec` on the grid. -#[derive(Default, Debug, Clone, PartialEq, Eq)] +#[derive(Default, Debug, Clone, PartialEq, Eq, Hash)] pub struct Extras { pub zerowidth: Vec, pub hyperlink: Option,