diff --git a/crates/doc/src/id_index.rs b/crates/doc/src/id_index.rs new file mode 100644 index 000000000..da36e9520 --- /dev/null +++ b/crates/doc/src/id_index.rs @@ -0,0 +1,85 @@ +//! A cache of every node's index path, so a lookup by id checks one path instead of walking the +//! tree. +//! +//! The tree is edited in many places through public fields, so the cache can't be kept in step +//! with it. It is checked instead: a cached path is used only while the node at that path still +//! has the id (a check as deep as the tree). A path that no longer checks out rebuilds the cache, +//! and an id the cache doesn't have (deleted, or added since) is looked up by walking, as before. +//! An id that appears more than once (a damaged file) is never cached, so lookups keep returning +//! its first place in tree order. +//! +//! The one case a cached lookup can answer differently from a walk: a second node with an +//! existing id put in the tree *after* the cache was built, earlier in tree order than the first. +//! The lookup then returns the original node (also one with that id) instead of the new one. The +//! app never makes such a node (paste and duplicate give new ids, undo restores whole trees); +//! duplicated ids come from damaged files, which are cached afresh when opened. + +use std::collections::HashMap; +use std::sync::{Arc, RwLock}; + +use crate::{Node, NodeId}; + +/// Each id's index path; `None` for an id that appears more than once. +pub(crate) type Paths = HashMap>>; + +/// Ids found by walking because the cache didn't have them, before the cache is rebuilt: each +/// rebuild costs a few walks, so rebuilding only after a few keeps a lookup's cost within a small +/// factor of a walk whatever the edit pattern (say, add an object, look it up, again and again). +const MISSES_PER_REBUILD: u32 = 4; + +/// The cache, shared by clones of a document (each checks every path it uses against its own tree). +#[derive(Default)] +pub struct IdIndex(RwLock>>, std::sync::atomic::AtomicU32); + +impl IdIndex { + /// The current cache, if one was built. + pub(crate) fn get(&self) -> Option> { + self.0.read().ok().and_then(|g| g.clone()) + } + + /// Rebuild the cache from `layers` (one walk) and return it. + pub(crate) fn rebuild(&self, layers: &[Arc]) -> Arc { + fn walk(nodes: &[Arc], path: &mut Vec, out: &mut Paths) { + for (i, n) in nodes.iter().enumerate() { + path.push(i); + out.entry(n.id).and_modify(|e| *e = None).or_insert_with(|| Some(path.clone())); + if let Some(ch) = n.children() { + walk(ch, path, out); + } + path.pop(); + } + } + let mut out = Paths::new(); + walk(layers, &mut Vec::new(), &mut out); + let out = Arc::new(out); + if let Ok(mut g) = self.0.write() { + *g = Some(out.clone()); + } + self.1.store(0, std::sync::atomic::Ordering::Relaxed); + out + } + + /// Note an id found by walking that the cache didn't have; true when it's time to rebuild. + pub(crate) fn missed(&self) -> bool { + self.1.fetch_add(1, std::sync::atomic::Ordering::Relaxed) + 1 >= MISSES_PER_REBUILD + } +} + +impl Clone for IdIndex { + fn clone(&self) -> Self { + IdIndex(RwLock::new(self.get()), Default::default()) + } +} + +/// A cache, not document content: two documents are equal whatever their caches hold. +impl PartialEq for IdIndex { + fn eq(&self, _: &Self) -> bool { + true + } +} + +impl std::fmt::Debug for IdIndex { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str("IdIndex") + } +} diff --git a/crates/doc/src/lib.rs b/crates/doc/src/lib.rs index 7825de13e..6e529106a 100644 --- a/crates/doc/src/lib.rs +++ b/crates/doc/src/lib.rs @@ -13,6 +13,7 @@ pub mod cmyk; pub mod corners; pub mod graph; pub mod hit; +mod id_index; pub mod inks; mod inline; pub use inline::{SYMBOL_HALF, SYMBOL_SIZES}; @@ -658,6 +659,9 @@ pub struct Document { /// state, never saved. #[serde(skip)] pub puppet: Option>, + /// Where each node is in the tree, for lookups by id ([`id_index`]): a cache, never saved. + #[serde(skip)] + index: id_index::IdIndex, } fn ppi72() -> f64 { @@ -729,6 +733,7 @@ impl Document { print_setup: None, assets: vec![], puppet: None, + index: Default::default(), }; let id = d.alloc_id(); d.layers.push(Arc::new(Node::layer(id, "Layer 1", LayerColor::Preset(0)))); @@ -758,24 +763,37 @@ impl Document { /// Find a node anywhere in the tree. pub fn node(&self, id: NodeId) -> Option<&Node> { - fn find(nodes: &[Arc], id: NodeId) -> Option<&Node> { - for n in nodes { - if n.id == id { - return Some(n); - } - if let Some(ch) = n.children() - && let Some(f) = find(ch, id) - { - return Some(f); - } - } - None - } - find(&self.layers, id) + self.index_path(id).and_then(|p| self.node_at(&p)) } - /// Index path from the top-level layer list to `id` (e.g. `[0, 3, 1]`). + /// Index path from the top-level layer list to `id` (e.g. `[0, 3, 1]`): the first place in + /// tree order where `id` appears. pub fn index_path(&self, id: NodeId) -> Option> { + // The cached path, while the node there still has the id ([`id_index`]). + let cached = self.index.get(); + match cached.as_ref().and_then(|m| m.get(&id)) { + Some(Some(p)) if self.node_at(p).is_some_and(|n| n.id == id) => return Some(p.clone()), + Some(None) => return self.walk_index_path(id), + Some(Some(_)) => { + // Stale: the tree changed. Rebuild, then answer from it (what a walk gives now). + return match self.index.rebuild(&self.layers).get(&id) { + Some(Some(p)) => Some(p.clone()), + Some(None) => self.walk_index_path(id), + None => None, + }; + } + None => {} + } + // Not cached: deleted, added since, or no cache yet. + let found = self.walk_index_path(id); + if cached.is_none() || (found.is_some() && self.index.missed()) { + self.index.rebuild(&self.layers); + } + found + } + + /// [`Document::index_path`] by walking the tree. + fn walk_index_path(&self, id: NodeId) -> Option> { fn find(nodes: &[Arc], id: NodeId, path: &mut Vec) -> bool { for (i, n) in nodes.iter().enumerate() { path.push(i); @@ -818,10 +836,23 @@ impl Document { } false } - let want: HashSet = ids.into_iter().collect(); + let mut want: HashSet = ids.into_iter().collect(); let mut out = HashMap::with_capacity(want.len()); + // Cached paths that still check out ([`id_index`]); the rest take one walk. For a good + // share of the document, one walk is quicker than checking each cached path. + if let Some(cache) = self.index.get().filter(|c| want.len().saturating_mul(8) < c.len()) { + want.retain(|id| match cache.get(id) { + Some(Some(p)) if self.node_at(p).is_some_and(|n| n.id == *id) => { + out.insert(*id, p.clone()); + false + } + _ => true, + }); + } if !want.is_empty() { - walk(&self.layers, &want, &mut Vec::new(), &mut out); + let mut rest = HashMap::with_capacity(want.len()); + walk(&self.layers, &want, &mut Vec::new(), &mut rest); + out.extend(rest); } out } diff --git a/crates/doc/src/tests_lookups.rs b/crates/doc/src/tests_lookups.rs index 3dff6215f..e42d2468b 100644 --- a/crates/doc/src/tests_lookups.rs +++ b/crates/doc/src/tests_lookups.rs @@ -78,3 +78,79 @@ fn batched_lookups_match_one_at_a_time() { assert_eq!(serde_json::to_string(&a).ok(), serde_json::to_string(&b).ok(), "seed {seed}"); } } + +/// What a walk of the tree gives for every id, checked against the cached lookups. +fn agree(d: &Document, ids: &[NodeId], what: &str) { + agree_except(d, ids, None, what); +} + +/// [`agree`], except for `dup`, an id given a second node after the cache was built: lookups of it +/// may return either of its nodes (see `id_index`), but always one with that id. +fn agree_except(d: &Document, ids: &[NodeId], dup: Option, what: &str) { + if let Some(dup) = dup { + assert_eq!(d.node(dup).map(|n| n.id), Some(dup), "{what}: node {dup:?} has its id"); + } + let ids: Vec = ids.iter().copied().filter(|id| Some(*id) != dup).collect(); + let ids = &ids[..]; + for id in ids { + let walked = d.walk_index_path(*id); + assert_eq!(d.index_path(*id), walked, "{what}: index_path {id:?}"); + let by_walk = walked.as_deref().and_then(|p| d.node_at(p)).map(|n| n as *const Node); + assert_eq!(d.node(*id).map(|n| n as *const Node), by_walk, "{what}: node {id:?}"); + } + let batched = d.index_paths(ids.iter().copied()); + for id in ids { + assert_eq!(batched.get(id).cloned(), d.walk_index_path(*id), "{what}: index_paths {id:?}"); + } +} + +/// The id cache never answers differently from a walk, however the tree is changed behind its +/// back (through the public fields), including in a clone that shares the cache. +#[test] +fn cached_lookups_follow_any_edit_of_the_tree() { + for seed in 1..30 { + let (mut d, ids) = tree(seed); + agree(&d, &ids, "fresh"); + let snapshot = d.clone(); // shares the cache + // Insert at the front of the first layer: every path in it shifts. + let extra = d.alloc_id(); + let leaf = Arc::new(Node::path(extra, shapes::rectangle(Rect::new(0.0, 0.0, 1.0, 1.0)), Appearance::default_art())); + if let Some(ch) = Arc::make_mut(&mut d.layers[0]).children_mut() { + ch.insert(0, leaf.clone()); + } + let mut all = ids.clone(); + all.push(extra); + agree(&d, &all, "inserted at the front"); + // Remove a child, swap two, and move one into another layer. + if let Some(ch) = Arc::make_mut(&mut d.layers[1]).children_mut() + && ch.len() > 3 + { + ch.remove(1); + ch.swap(0, 2); + } + let moved = Arc::make_mut(&mut d.layers[2]).children_mut().and_then(|ch| (!ch.is_empty()).then(|| ch.remove(0))); + if let Some(m) = moved + && let Some(ch) = Arc::make_mut(&mut d.layers[0]).children_mut() + { + ch.push(m); + } + agree(&d, &all, "removed, swapped and moved"); + // A new duplicate of an existing id, earlier in tree order than the original. + let dup = ids[ids.len() / 3]; + let copy = Arc::new(Node::path(dup, shapes::rectangle(Rect::new(0.0, 0.0, 2.0, 2.0)), Appearance::default_art())); + if let Some(ch) = Arc::make_mut(&mut d.layers[0]).children_mut() { + ch.insert(0, copy); + } + agree_except(&d, &all, Some(dup), "a duplicate inserted earlier"); + // Delete a whole layer, then look everything up again, missing ids included. + d.layers.remove(1); + agree_except(&d, &all, Some(dup), "a layer deleted"); + // A rebuilt cache (as after loading a file) knows the duplicate and walks for it. + let mut fresh = d.clone(); + fresh.layers = d.layers.clone(); + fresh.index = Default::default(); + agree(&fresh, &all, "a damaged tree, cached afresh"); + // The clone made before all this still answers for its own tree. + agree(&snapshot, &ids, "the earlier clone"); + } +}