From 646ac60b75d8f8700556c419c1ffb9a7568007cb Mon Sep 17 00:00:00 2001 From: saimskywalker <15120878+saimskywalker@users.noreply.github.com> Date: Sat, 31 Oct 2026 01:26:59 +0800 Subject: [PATCH 1/2] M8: an edit checks only the layers it changed for out-of-range positions and sizes (#1672) check_finite_geometry went over every layer of the document after every command, so each edit cost time in proportion to the file, not the edit. It now walks Document::changed_ids against the version the command started from; the imbl diff skips what the two versions share. 100 renames among 200,000 layers: ~0.60 s before, ~0.02 s after. --- crates/engine/src/finite_geometry_tests.rs | 59 ++++++++++++++++++++++ crates/engine/src/session/exec.rs | 5 +- 2 files changed, 63 insertions(+), 1 deletion(-) diff --git a/crates/engine/src/finite_geometry_tests.rs b/crates/engine/src/finite_geometry_tests.rs index 416b250c8..a8b552a73 100644 --- a/crates/engine/src/finite_geometry_tests.rs +++ b/crates/engine/src/finite_geometry_tests.rs @@ -38,3 +38,62 @@ fn huge_scale_is_refused_and_the_file_still_reopens() { assert!(n.width.is_finite() && n.height.is_finite(), "{} x {}", n.width, n.height); assert_roundtrips(&s); } + +/// A settled document of `groups` groups of `per` rectangles each, and one of the rectangles. +fn large(groups: usize, per: usize) -> (figcraft_doc::Document, crate::NodeId) { + use figcraft_doc::NodeKind; + let mut doc = figcraft_doc::Document::new("Large"); + let page = doc.first_page().unwrap(); + let mut last = None; + for g in 0..groups { + let group = doc.create(page, NodeKind::Group, "group", 0.0, g as f64 * 20.0, 10.0, 10.0).unwrap(); + for i in 0..per { + last = Some(doc.create(group, NodeKind::Rectangle, "rect", i as f64 * 20.0, 0.0, 10.0, 10.0).unwrap()); + } + } + doc.take_dirty(); + (doc, last.unwrap()) +} + +/// Seconds `renames` renames of one layer take, one command each. +fn rename_seconds(s: &mut Session, id: crate::NodeId, renames: usize) -> f64 { + let start = std::time::Instant::now(); + for i in 0..renames { + s.execute("node.set", &json!({"ids": [id], "props": {"name": format!("rect {i}")}})).unwrap(); + } + start.elapsed().as_secs_f64() +} + +#[test] +fn an_edit_checks_only_what_it_changed_not_the_whole_document() { + // #1672: the check went over every layer of the file after every edit, so a batch slowed down + // with the size of the file, not of the edit. + let (small, small_id) = large(1, 10); + let (big, big_id) = large(2000, 100); + let mut s = Session::with_document(small); + let mut b = Session::with_document(big); + // Warm up both (first-command bookkeeping), then time the same renames. + rename_seconds(&mut s, small_id, 3); + rename_seconds(&mut b, big_id, 3); + let (in_small, in_big) = (rename_seconds(&mut s, small_id, 100), rename_seconds(&mut b, big_id, 100)); + assert_eq!(b.doc().unwrap().node(big_id).unwrap().name, "rect 99"); + // 200,000 layers against 10: what grows with the document is a log factor, not a scan (with + // the scan these 100 renames took about 0.6 s, without it about 0.02 s). + assert!(in_big < in_small * 20.0 + 0.25, "100 renames took {in_big:.3} s among 200,000 layers, {in_small:.3} s among 10"); +} + +#[test] +fn a_layer_already_out_of_range_does_not_block_edits_to_others() { + // A damaged file can hold a layer with a non-finite size; edits to other layers still go + // through, and an edit that touches the damaged layer is refused. + let (mut doc, id) = large(2, 3); + let page = doc.first_page().unwrap(); + let broken = doc.create(page, figcraft_doc::NodeKind::Rectangle, "broken", 0.0, 0.0, 10.0, 10.0).unwrap(); + doc.node_mut(broken).unwrap().width = f64::NAN; + doc.take_dirty(); + let mut s = Session::with_document(doc); + assert!(s.execute("node.set", &json!({"ids": [id], "props": {"name": "renamed"}})).is_ok()); + let moved = s.execute("transform.move", &json!({"ids": [broken], "dx": 5, "dy": 0})); + let err = moved.expect_err("an edit of the broken layer is refused").to_string(); + assert!(err.contains("broken"), "{err}"); +} diff --git a/crates/engine/src/session/exec.rs b/crates/engine/src/session/exec.rs index 050244813..61fa8a42d 100644 --- a/crates/engine/src/session/exec.rs +++ b/crates/engine/src/session/exec.rs @@ -179,7 +179,10 @@ impl Session { } let broken = |n: &Node| !(n.transform.as_coeffs().iter().all(|c| c.is_finite()) && n.width.is_finite() && n.height.is_finite()); let untouched = |n: &Node| bdoc.node_arc(n.id).zip(ds.doc.node_arc(n.id)).is_some_and(|(a, b)| Arc::ptr_eq(a, b)); - match ds.doc.nodes().find(|n| broken(n) && !untouched(n)) { + // Only the layers the command added or changed: the diff skips what the two versions + // share, so the check costs about the size of the edit, not of the document (#1672). + let changed = ds.doc.changed_ids(bdoc, usize::MAX).unwrap_or_default(); + match changed.iter().filter_map(|i| ds.doc.node(*i)).find(|n| broken(n) && !untouched(n)) { Some(n) => Err(bad(id, format!("\"{}\" would get a position or size beyond the number range", n.name))), None => Ok(()), } From 7ce19ebe382e54311cf2209ff077af547cc2fc55 Mon Sep 17 00:00:00 2001 From: saimskywalker <15120878+saimskywalker@users.noreply.github.com> Date: Sat, 31 Oct 2026 01:46:30 +0800 Subject: [PATCH 2/2] M8: test the finite-geometry check by the layers it visits, not by time (#1672) The timing assertion could fail under load. finite_check_candidates is now its own function, and the test checks that a rename among 200,000 layers looks at only a handful of them. --- crates/engine/src/finite_geometry_tests.rs | 30 ++++++---------------- crates/engine/src/session.rs | 2 ++ crates/engine/src/session/exec.rs | 12 ++++++--- 3 files changed, 18 insertions(+), 26 deletions(-) diff --git a/crates/engine/src/finite_geometry_tests.rs b/crates/engine/src/finite_geometry_tests.rs index a8b552a73..52ba2a575 100644 --- a/crates/engine/src/finite_geometry_tests.rs +++ b/crates/engine/src/finite_geometry_tests.rs @@ -55,31 +55,17 @@ fn large(groups: usize, per: usize) -> (figcraft_doc::Document, crate::NodeId) { (doc, last.unwrap()) } -/// Seconds `renames` renames of one layer take, one command each. -fn rename_seconds(s: &mut Session, id: crate::NodeId, renames: usize) -> f64 { - let start = std::time::Instant::now(); - for i in 0..renames { - s.execute("node.set", &json!({"ids": [id], "props": {"name": format!("rect {i}")}})).unwrap(); - } - start.elapsed().as_secs_f64() -} - #[test] fn an_edit_checks_only_what_it_changed_not_the_whole_document() { // #1672: the check went over every layer of the file after every edit, so a batch slowed down - // with the size of the file, not of the edit. - let (small, small_id) = large(1, 10); - let (big, big_id) = large(2000, 100); - let mut s = Session::with_document(small); - let mut b = Session::with_document(big); - // Warm up both (first-command bookkeeping), then time the same renames. - rename_seconds(&mut s, small_id, 3); - rename_seconds(&mut b, big_id, 3); - let (in_small, in_big) = (rename_seconds(&mut s, small_id, 100), rename_seconds(&mut b, big_id, 100)); - assert_eq!(b.doc().unwrap().node(big_id).unwrap().name, "rect 99"); - // 200,000 layers against 10: what grows with the document is a log factor, not a scan (with - // the scan these 100 renames took about 0.6 s, without it about 0.02 s). - assert!(in_big < in_small * 20.0 + 0.25, "100 renames took {in_big:.3} s among 200,000 layers, {in_small:.3} s among 10"); + // with the size of the file, not of the edit. Count the layers it looks at instead of timing it. + let (big, id) = large(2000, 100); + let mut s = Session::with_document(big); + let before = s.doc().unwrap().clone(); + s.execute("node.set", &json!({"ids": [id], "props": {"name": "renamed"}})).unwrap(); + let checked = crate::session::finite_check_candidates(s.doc().unwrap(), &before); + assert!(checked.contains(&id), "the renamed layer is checked: {checked:?}"); + assert!(checked.len() <= 10, "a rename among 200,000 layers checks {} of them", checked.len()); } #[test] diff --git a/crates/engine/src/session.rs b/crates/engine/src/session.rs index 99101dc2b..113b75cdb 100644 --- a/crates/engine/src/session.rs +++ b/crates/engine/src/session.rs @@ -10,5 +10,7 @@ use super::{ mod access; pub use access::Forward; mod exec; +#[cfg(test)] +pub(crate) use exec::finite_check_candidates; mod new; mod undo; diff --git a/crates/engine/src/session/exec.rs b/crates/engine/src/session/exec.rs index 61fa8a42d..097f44c78 100644 --- a/crates/engine/src/session/exec.rs +++ b/crates/engine/src/session/exec.rs @@ -16,6 +16,13 @@ fn command_carries_import_bytes(id: &str, params: &Value) -> bool { } } +/// The layers `check_finite_geometry` looks at: only those the command added or changed. The diff +/// skips what the two versions share, so the check costs about the size of the edit, not of the +/// document (#1672). +pub(crate) fn finite_check_candidates(doc: &Document, before: &Document) -> Vec { + doc.changed_ids(before, usize::MAX).unwrap_or_default() +} + impl Session { /// Append a live event with the active document's identity (nothing without one). Inside a /// [`Session::grouped`] call the event is buffered and committed only when the group succeeds. @@ -179,10 +186,7 @@ impl Session { } let broken = |n: &Node| !(n.transform.as_coeffs().iter().all(|c| c.is_finite()) && n.width.is_finite() && n.height.is_finite()); let untouched = |n: &Node| bdoc.node_arc(n.id).zip(ds.doc.node_arc(n.id)).is_some_and(|(a, b)| Arc::ptr_eq(a, b)); - // Only the layers the command added or changed: the diff skips what the two versions - // share, so the check costs about the size of the edit, not of the document (#1672). - let changed = ds.doc.changed_ids(bdoc, usize::MAX).unwrap_or_default(); - match changed.iter().filter_map(|i| ds.doc.node(*i)).find(|n| broken(n) && !untouched(n)) { + match finite_check_candidates(&ds.doc, bdoc).iter().filter_map(|i| ds.doc.node(*i)).find(|n| broken(n) && !untouched(n)) { Some(n) => Err(bad(id, format!("\"{}\" would get a position or size beyond the number range", n.name))), None => Ok(()), }