diff --git a/crates/collab/src/diff.rs b/crates/collab/src/diff.rs index 016dc5dd0..91e156155 100644 --- a/crates/collab/src/diff.rs +++ b/crates/collab/src/diff.rs @@ -95,7 +95,8 @@ pub fn diff(from: &Replica, to: &Document) -> Result { let mut changes = fields; let new: BTreeSet = added.iter().copied().collect(); // Creations, parents before children. - added.sort_by_cached_key(|id| (depth(to, *id), *id)); + let mut memo = BTreeMap::new(); + added.sort_by_cached_key(|id| (depth(to, *id, &mut memo), *id)); for id in &added { if let Some(n) = to.node(*id) { changes.push(create_change(n, replica.key(*id))?); @@ -121,18 +122,30 @@ pub fn diff(from: &Replica, to: &Document) -> Result { Ok(Diff { changes, replica }) } -/// Depth of `id` in `doc` (pages are 0). -fn depth(doc: &Document, id: NodeId) -> usize { - let mut d = 0; +/// Depth of `id` in `doc` (pages are 0). `memo` holds the depths found so far, so a deep chain of +/// new layers is walked once rather than once per layer. +fn depth(doc: &Document, id: NodeId, memo: &mut BTreeMap) -> usize { + let mut chain = vec![id]; + let mut base = 0; let mut cur = doc.node(id).and_then(|n| n.parent); while let Some(p) = cur { - d += 1; - if d > 100_000 { + if let Some(d) = memo.get(&p) { + base = *d + 1; break; } + // An acyclic chain is no longer than the document: this only stops a cycle. + if chain.len() > doc.node_count() { + break; + } + chain.push(p); cur = doc.node(p).and_then(|n| n.parent); } - d + // `chain` runs from `id` up; the last entry's depth is `base` when it ends at a root or a memo hit. + let top = chain.len(); + for (i, c) in chain.iter().enumerate() { + memo.insert(*c, base + (top - 1 - i)); + } + memo.get(&id).copied().unwrap_or(0) } /// New keys for the children `kids` (in their new order) whose `old` key no longer fits: the @@ -196,3 +209,21 @@ fn increasing_run(keys: &[Option<&str>]) -> Vec { } kept } + +#[cfg(test)] +mod depth_tests { + use super::*; + use figcraft_doc::NodeKind; + + #[test] + fn a_chain_longer_than_a_fixed_bound_is_walked_whole() { + let mut doc = Document::new("File"); + let mut parent = doc.first_page().unwrap(); + let mut last = parent; + for i in 0..100_050 { + last = doc.create(parent, NodeKind::Frame, &format!("F{i}"), 0.0, 0.0, 1.0, 1.0).unwrap(); + parent = last; + } + assert_eq!(depth(&doc, last, &mut BTreeMap::new()), 100_050); + } +} diff --git a/crates/collab/src/diff_tests.rs b/crates/collab/src/diff_tests.rs index 8c6a1211c..bb81d0b1b 100644 --- a/crates/collab/src/diff_tests.rs +++ b/crates/collab/src/diff_tests.rs @@ -310,3 +310,14 @@ fn many_sibling_placements_keep_the_order_of_counting_them() { assert_eq!(r.document().node(p).unwrap().children, kids[&p], "children of {p}"); } } + +#[test] +fn a_chain_of_new_nested_layers_is_created_parents_first() { + let (a, _, frame, _) = setup(); + let mut b = a.document().clone(); + let mut parent = frame; + for i in 0..400 { + parent = b.create(parent, NodeKind::Frame, &format!("F{i}"), 0.0, 0.0, 10.0, 10.0).unwrap(); + } + assert_eq!(round_trip(&a, &b).iter().filter(|c| matches!(c, Change::Create { .. })).count(), 400); +} diff --git a/crates/collab/src/replica.rs b/crates/collab/src/replica.rs index d14676a01..547f0f59b 100644 --- a/crates/collab/src/replica.rs +++ b/crates/collab/src/replica.rs @@ -236,7 +236,9 @@ impl Replica { self.doc.insert_page(node).map_err(doc_err)?; } (false, Some(p)) => { - let at = self.slot(&self.doc.get(*p).map_err(doc_err)?.children, *id, position); + // A new id sits in no sibling list yet: no scan for it, just the binary search. + let siblings = &self.doc.get(*p).map_err(doc_err)?.children; + let at = siblings.partition_point(|c| (self.key(*c), *c) < (position.as_str(), *id)); self.doc.insert(node, *p, Some(at)).map_err(doc_err)?; } _ => return Err(Error::Invalid("pages, and only pages, have no parent".into())),