From 36bdf4ea3c8fec7332ca5209908e49030fa87f26 Mon Sep 17 00:00:00 2001 From: kaltenbrunnermarkus <153593707+kaltenbrunnermarkus@users.noreply.github.com> Date: Sun, 1 Nov 2026 10:13:17 +0000 Subject: [PATCH] Sketch drag: drop the second document copy per frame (#10507) * claim: Sketch drag: drop the second document copy per frame * Sketch drag: drop the second document copy per frame edit_solved copied the document to edit it, then stored it with `*s.doc_mut() = doc`. The command's undo snapshot still holds the old document, so doc_mut copied the whole design a second time only to overwrite it. Store the edited document with Session::set_doc instead, which forgets the same caches and puts it in a new Arc. The design lands at a new address as before and the command's end records it for the crash journal (#7502). A live drag frame on a design of 200 sketches (20k curves) drops from 19.2 ms to 9.4 ms; 50 sketches: 2.3 ms to 1.7 ms. Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Markus Kaltenbrunner Co-authored-by: Claude Opus 5.5 --- crates/engine/src/cmd/sketch.rs | 2 +- .../engine/src/cmd/sketch_drag_live_tests.rs | 17 +++++ crates/engine/src/cmd/sketch_perf_tests.rs | 75 +++++++++++++++++++ crates/engine/src/crash_tests.rs | 30 ++++++++ crates/engine/src/lib.rs | 27 ++++++- crates/engine/src/tests.rs | 18 +++++ 6 files changed, 164 insertions(+), 5 deletions(-) diff --git a/crates/engine/src/cmd/sketch.rs b/crates/engine/src/cmd/sketch.rs index 120c21a37f..59e6c59927 100644 --- a/crates/engine/src/cmd/sketch.rs +++ b/crates/engine/src/cmd/sketch.rs @@ -325,7 +325,7 @@ pub(super) fn edit_solved( }; sketch_store::checked(&sk)?; sketch_store::write(&mut doc, id, sk)?; - *s.doc_mut() = doc; + s.set_doc(doc); Ok((out, rep)) } diff --git a/crates/engine/src/cmd/sketch_drag_live_tests.rs b/crates/engine/src/cmd/sketch_drag_live_tests.rs index 2e46b658dc..73a6721a2e 100644 --- a/crates/engine/src/cmd/sketch_drag_live_tests.rs +++ b/crates/engine/src/cmd/sketch_drag_live_tests.rs @@ -126,6 +126,23 @@ fn a_live_frame_solves_once_and_leaves_the_features_after_the_sketch_stale() { assert!(s.model.state().bodies.first().expect("a body").body.same(&body), "the body is not rebuilt"); } +/// A frame copies the design once, to edit it: its undo step holds the design as it was, and a +/// second copy for storing the edited one would double the work on a large design. +#[test] +fn a_live_frame_copies_the_design_once_and_its_undo_step_keeps_the_design_before() { + let (mut s, _) = extruded(); + let before = s.doc.clone(); + let undo = s.undo.len(); + crate::tests::doc_copies::take(); + frames(&mut s, 1, true); + assert_eq!(crate::tests::doc_copies::take(), 0, "the edited design is stored, not copied again"); + assert!(corner(&s).dist(solidcraft_geom::Vec2::new(4.0, 5.0)) < 1e-9); + assert_eq!(s.undo.len(), undo + 1); + assert!(s.undo.last().is_some_and(|u| std::sync::Arc::ptr_eq(&u.doc, &before)), "undo goes back to the design before"); + run(&mut s, "edit.undo", json!({})); + assert!(std::sync::Arc::ptr_eq(&s.doc, &before)); +} + #[test] fn the_drag_end_evaluates_once_to_the_scripted_drags_result() { let (mut live, extrude) = extruded(); diff --git a/crates/engine/src/cmd/sketch_perf_tests.rs b/crates/engine/src/cmd/sketch_perf_tests.rs index 0f97b4acb2..220d98f95c 100644 --- a/crates/engine/src/cmd/sketch_perf_tests.rs +++ b/crates/engine/src/cmd/sketch_perf_tests.rs @@ -83,3 +83,78 @@ fn sketch_perf() { ); } } + +/// Fill sketch `id` with `n` constrained rectangles, each with a circle inside. +fn fill(doc: &mut solidcraft_doc::Document, id: u64, n: usize, x0: f64) { + use solidcraft_geom::Vec2; + use solidcraft_sketch::ConstraintKind::*; + let Ok(sk) = doc.sketch_mut(id) else { return }; + for k in 0..n { + let (x, y) = (x0 + k as f64 * 30.0, 0.0); + let p: Vec = [(0.0, 0.0), (20.0, 0.3), (20.2, 15.0), (-0.1, 15.1)] + .iter() + .map(|(dx, dy)| sk.add_point(Vec2::new(x + dx, y + dy), None).unwrap()) + .collect(); + let l: Vec = (0..4).map(|i| sk.add_line_pts(p[i], p[(i + 1) % 4], None).unwrap()).collect(); + sk.add_constraint(Horizontal { l: l[0] }, None).unwrap(); + sk.add_constraint(Horizontal { l: l[2] }, None).unwrap(); + sk.add_constraint(Vertical { l: l[1] }, None).unwrap(); + sk.add_constraint(Vertical { l: l[3] }, None).unwrap(); + sk.add_constraint(Length { l: l[0], value: 20.0 }, None).unwrap(); + sk.add_constraint(Length { l: l[1], value: 15.0 }, None).unwrap(); + sk.add_circle(Vec2::new(x + 10.0, y + 7.5), 3.0, None, None).unwrap(); + } +} + +/// A large design (`sketches` sketches of `rects` rectangles, `params` parameters), the first +/// sketch (`first` rectangles) open for editing; per live drag frame: execute time and the +/// time of one copy of the document. +#[test] +#[ignore] +fn sketch_drag_frame_perf() { + let cases: [(usize, usize, usize, usize); 3] = [(50, 10, 200, 2), (200, 20, 500, 2), (200, 20, 500, 50)]; + for (sketches, rects, params, first) in cases { + let mut s = Session::default(); + for i in 0..params { + s.execute("parameters.add", &json!({"name": format!("p{i}"), "expression": format!("{i} mm")})).unwrap(); + } + for k in 0..sketches { + s.execute("sketch.create", &json!({"plane": "XY"})).unwrap(); + let id = s.active_sketch.unwrap(); + let mut doc = (*s.doc).clone(); + fill(&mut doc, id, if k == 0 { first } else { rects }, 0.0); + *s.doc_mut() = doc; + s.execute("sketch.finish", &json!({})).unwrap(); + } + s.execute("sketch.edit", &json!({"sketch": "Sketch1"})).unwrap(); + let curves: usize = s.doc.features.iter().filter_map(|f| s.doc.sketch(f.id).ok()).map(|k| k.curves.len()).sum(); + let json_len = serde_json::to_string(&*s.doc).map(|j| j.len()).unwrap_or(0); + let frames = 60; + let mut frame_ms = Vec::new(); + for f in 0..frames { + let to = [1.0 + f as f64 * 0.05, 1.0 + f as f64 * 0.03]; + let t = Instant::now(); + s.execute("sketch.drag", &json!({"entity": "p1", "from": [0, 0], "to": to, "live": true})).unwrap(); + frame_ms.push(ms(t)); + } + s.execute("sketch.drag_end", &json!({})).ok(); + let digest = { + use std::hash::{Hash, Hasher}; + let mut h = std::collections::hash_map::DefaultHasher::new(); + serde_json::to_string(&*s.doc).unwrap_or_default().hash(&mut h); + format!("{:016x}", h.finish()) + }; + frame_ms.sort_by(f64::total_cmp); + let t = Instant::now(); + for _ in 0..frames { + std::hint::black_box((*s.doc).clone()); + } + let copy = ms(t) / frames as f64; + let frame = frame_ms[frames / 2]; + eprintln!( + "DRAGPERF sketches={sketches} rects={rects} first={first} params={params} features={} curves={curves} json={json_len}B: frame median {frame:.3} ms, one copy {copy:.3} ms ({:.1} %), design {digest}", + s.doc.features.len(), + 100.0 * copy / frame + ); + } +} diff --git a/crates/engine/src/crash_tests.rs b/crates/engine/src/crash_tests.rs index 0c4608562f..9be4e06092 100644 --- a/crates/engine/src/crash_tests.rs +++ b/crates/engine/src/crash_tests.rs @@ -130,6 +130,29 @@ fn child() { assert_eq!(read_live_journal(state).base, json!({"file":path})); println!("PREPARED_JOURNAL_OK"); } + // Live sketch drag frames store the edited design without copying it again: each + // frame is journaled, and the next frame does not take the design for an outside edit. + "sketch_drag" => { + s.execute("sketch.create", &json!({"plane": "XY"})).unwrap(); + s.execute("sketch.rectangle.two_point", &json!({"p0": [5, 5], "p1": [25, 20]})).unwrap(); + for k in 1..=3 { + let (from, to) = ([6.0 - k as f64, 5.0], [5.0 - k as f64, 5.0]); + s.execute("sketch.drag", &json!({"entity": "l1.start", "from": from, "to": to, "live": true})).unwrap(); + } + s.execute("sketch.drag_end", &json!({})).unwrap(); + let journal = read_live_journal(STATE.get().unwrap()); + assert_eq!(journal.base, json!("new"), "the drag frames continue the replay"); + // The drag's end only evaluates the model: it changes nothing a replay needs. + let ids: Vec<&str> = journal.replay.iter().filter_map(|c| c["command"].as_str()).collect(); + assert_eq!(ids, ["solid.box", "sketch.create", "sketch.rectangle.two_point", "sketch.drag", "sketch.drag", "sketch.drag"]); + let mut replay = Session::default(); + for command in &journal.replay { + replay.execute(command["command"].as_str().unwrap(), &command["params"]).unwrap(); + } + let id = s.active_sketch.unwrap(); + assert_eq!(replay.doc.sketch(id).ok(), s.doc.sketch(id).ok(), "the replay drags the sketch to the same place"); + println!("SKETCH_DRAG_JOURNAL_OK"); + } "render" => { s.execute("render.settings.set", &json!({"name":"Studio","set":{"exposure_ev":2}})).unwrap(); s.execute("render.settings.get", &json!({"name":"Studio"})).unwrap(); @@ -212,6 +235,13 @@ fn a_read_only_command_keeps_the_replay_after_body_names_are_pinned() { let _ = fs::remove_dir_all(directory); } +#[test] +fn live_sketch_drag_frames_keep_the_replay() { + let directory = std::env::temp_dir().join(format!("solidcraft-crash-test-sketch-drag-{}", std::process::id())); + let (output, ok) = run_child(&directory, "sketch_drag"); + assert!(ok && output.contains("SKETCH_DRAG_JOURNAL_OK"), "{output}"); +} + fn only_report(dir: &Path) -> (Summary, Value) { let all = list(dir); assert_eq!(all.len(), 1, "{all:?}"); diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 56b27f9598..e99e069738 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -932,16 +932,35 @@ impl Session { /// Mutable document (copy-on-write). pub fn doc_mut(&mut self) -> &mut Document { - // The placement index keeps the document alive with its answers; let it go before an - // in-place edit, so the edit stays in place (a shared handle would clone the document - // to a new address behind the crash journal's back, #7502) and no stale lookup outlives it. + self.forget_document_caches(); + #[cfg(test)] + if Arc::strong_count(&self.doc) > 1 { + tests::doc_copies::record(); + } + Arc::make_mut(&mut self.doc) + } + + /// Make `doc` the design, as `*doc_mut() = doc` does, without first copying a shared + /// document only to overwrite the copy: a command's undo step still holds the design it + /// started from, so `doc_mut` would copy all of it (every feature and sketch) on each + /// sketch drag frame. The design lands at a new address either way, and the command's end + /// tells the crash journal that address (#7502). + pub(crate) fn set_doc(&mut self, doc: Document) { + self.forget_document_caches(); + self.doc = Arc::new(doc); + } + + /// Before the document changes: the placement index keeps the document alive with its + /// answers; let it go, so an in-place edit stays in place (a shared handle would clone the + /// document to a new address behind the crash journal's back, #7502) and no stale lookup + /// outlives it. + fn forget_document_caches(&mut self) { if let Ok(mut index) = self.placement_index_cache.lock() { *index = None; } self.forget_consumed_sketches(); self.forget_dirty_answer(); self.model.forget_fingerprints(); - Arc::make_mut(&mut self.doc) } /// Prepared placements only: optional save caches must not compute a cold world. diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 34222e5713..540f634df4 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -6,6 +6,24 @@ use crate::*; mod mesh_convert; +/// Counts the copies `Session::doc_mut` makes of a shared document. +pub(crate) mod doc_copies { + use std::cell::Cell; + + thread_local! { + static COUNT: Cell = const { Cell::new(0) }; + } + + pub fn record() { + COUNT.with(|c| c.set(c.get() + 1)); + } + + /// The count since the last call. + pub fn take() -> usize { + COUNT.with(|c| c.replace(0)) + } +} + fn run(s: &mut Session, id: &str, p: Value) -> Value { match s.execute(id, &p) { Ok(v) => v,