diff --git a/src/app/update/mod.rs b/src/app/update/mod.rs index fe8519aa1..21e117a8d 100644 --- a/src/app/update/mod.rs +++ b/src/app/update/mod.rs @@ -265,13 +265,11 @@ impl OpenCADStudio { self.control_observe_user_message(&msg); let perf_started = crate::perf::enabled().then(Instant::now); let perf_label = perf_message_label(&msg); - // Geometry epoch before dispatch. When it moves, this message drew or - // changed something, and the latency the user feels runs from here to - // the frame that shows it — not to the end of this handler. Stamping - // the nav-perf sample makes `nav-prepare` / `nav-render` report that - // span, the same way they already do for pan and zoom. - let perf_epoch_before = perf_started - .map(|_| self.tabs[self.active_tab].scene.geometry_epoch); + // Keep the stable tab id because dispatch may switch or close tabs. + let perf_edit_before = perf_started.map(|started| { + let tab = &self.tabs[self.active_tab]; + (started, tab.id, tab.scene.geometry_epoch) + }); // A modal dialog must capture the keyboard the same way it already // captures the mouse. Otherwise keystrokes from the global key // subscription leak past the modal into the command line and fire as @@ -331,14 +329,15 @@ impl OpenCADStudio { self.otrack_active = None; self.otrack_kind = None; } - if let (Some(started), Some(before)) = (perf_started, perf_epoch_before) { - let tab = &self.tabs[self.active_tab]; - if tab.scene.geometry_epoch != before { - tab.scene.record_nav_perf_caused( - crate::scene::NavPerfOp::Edit, - perf_label, - started, - ); + if let Some((started, tab_id, before)) = perf_edit_before { + if let Some(tab) = self.tabs.iter().find(|tab| tab.id == tab_id) { + if tab.scene.geometry_epoch != before { + tab.scene.record_nav_perf_caused( + crate::scene::NavPerfOp::Edit, + perf_label, + started, + ); + } } } if let Some(started) = perf_started { diff --git a/src/scene/mod.rs b/src/scene/mod.rs index 9d62dd7f6..75bc719a5 100644 --- a/src/scene/mod.rs +++ b/src/scene/mod.rs @@ -437,9 +437,7 @@ pub struct PreparedOpenGeometry { pub tess_guard: u64, /// Loader block definitions; the receiving scene supplies its own epoch. pub block_defns: Vec<(u64, Arc)>, - /// The loader's draw-order map. Computing it walks all 1.17 M entities and - /// costs ~300 ms; the loader already paid that, and the receiving scene - /// used to pay it again on the first frame. + /// Loader-computed draw-order map, re-stamped by the receiving scene. pub(crate) draw_depths: Option, } @@ -1439,9 +1437,7 @@ pub(crate) enum NavPerfOp { Pan, Zoom, Rotate, - /// A message that changed geometry. Reported through the same line as - /// navigation because the question is the same one: how long from the - /// input to the frame that answers it. + /// A message that changed geometry before the next rendered frame. Edit, } @@ -1459,9 +1455,7 @@ impl NavPerfOp { #[derive(Clone, Copy, Debug)] pub(in crate::scene) struct NavPerfSample { pub(in crate::scene) op: NavPerfOp, - /// Which message caused it. An edit whose handler runs in under a - /// millisecond never reaches the `update` line's threshold, so without this - /// the log shows the latency but not what asked for it. + /// Message label that caused the sample. pub(in crate::scene) cause: &'static str, pub(in crate::scene) space: &'static str, pub(in crate::scene) mode: &'static str, @@ -1715,11 +1709,8 @@ pub struct Scene { /// constants. `[depth, half]` retains a fixed child sub-range for block /// composition. Full sort/layout/block changes rebuild the labels. draw_depth_cache: RefCell>, - /// Bumped only when the draw-order map is rebuilt from scratch, which is - /// the one case that can change an existing entity's depth — an - /// incremental insert allocates into the reserved label gap and leaves - /// every other label alone. Consumers that cached anything carrying a baked - /// depth compare this before reusing it. + /// Bumped when a full draw-order rebuild can move existing depths. + /// Cached uploads compare it before reusing baked depth values. draw_depth_generation: std::cell::Cell, /// Shared empty map for 3-D wireframe, where true depth wins instead of /// the entity submission order used by the optimized 2-D style. @@ -1869,16 +1860,8 @@ pub struct Scene { block_members_cache: RefCell>, /// Insert/Viewport/Block/BlockEnd handles omitted by the spatial index. unindexable_cache: RefCell)>>, - /// Distinct entity type names present in a layout, keyed by - /// (geometry_epoch, layout block). The status bar asks for this on every - /// frame to populate the selection-filter menu, but the answer only - /// changes when entities are added or removed. - /// `(epoch, block, present type names, the list handed to the UI)`. - /// - /// The set is kept alongside the list so an edit can be folded in without - /// rebuilding either: drawing a line into a drawing that already has lines - /// does not change the answer, and the walk that produces it visits every - /// entity the layout owns. + /// `(epoch, layout block, present type names, list exposed to the UI)`. + /// The set lets pure additions update without rebuilding the list. layout_type_names_cache: RefCell< Option<( u64, @@ -2286,11 +2269,7 @@ impl Scene { *self.resident_tess_memo.borrow_mut() = prepared.tess_memo; self.resident_tess_guard.set(prepared.tess_guard); } - // The draw-order map, on the same terms as the block definitions - // below: the document is the loader's own, unmodified since, so its - // order labels describe this scene exactly. Only the epoch is this - // scene's. Without this the first frame recomputed it — a walk of - // every entity in the drawing, ~300 ms, for a map that already existed. + // Reuse the loader's draw-order map with this scene's epoch. if let Some(mut depths) = prepared.draw_depths { depths.epoch = self.geometry_epoch; *self.draw_depth_cache.borrow_mut() = Some(depths); @@ -8887,12 +8866,7 @@ impl Scene { ) } - /// `resident_entity_visible` with the layer already resolved. - /// - /// `Table::get` normalises the name with `to_uppercase`, which allocates, - /// and the selection calls this once per candidate — 444 938 allocations on - /// the reproducer, ~440 ms, to look up a few hundred distinct layers. A - /// caller in a loop resolves each name once and passes the result here. + /// Visibility predicate with the layer resolved by the caller. fn resident_entity_visible_with_layer( &self, e: &EntityType, @@ -8995,32 +8969,12 @@ impl Scene { // Visibility test reused by both paths below — and by the resident // incremental patch, so a changed entity is included/excluded exactly as // a from-scratch build would (no divergence). - // `Table::get` normalises with `to_uppercase`, so every layer lookup - // allocates. The selection asks once per candidate and a drawing has a - // few hundred layers, so the name is resolved once and remembered. - // Keys borrow the entity's own layer string, which lives in the - // document for the whole call. + // Resolve each distinct layer name once for this candidate pass. type LayerRef<'a> = Option<&'a acadrust::tables::layer::Layer>; let layer_of: RefCell>> = RefCell::new(rustc_hash::FxHashMap::default()); - // Kill switch, so one binary can measure both paths on one drawing. - // Same shape as `OCS_NO_RESIDENT_MEMO`. - fn layer_memo_enabled() -> bool { - static EN: std::sync::OnceLock = std::sync::OnceLock::new(); - *EN.get_or_init(|| std::env::var("OCS_NO_LAYER_MEMO").is_err()) - } - let memo_on = layer_memo_enabled(); let visibility_ok = |e: &'doc EntityType| { let name = e.common().layer.as_str(); - if !memo_on { - return self.resident_entity_visible( - e, - block_handle, - frozen_layers, - annotation_scale_handle, - all_visible, - ); - } let layer = match layer_of.borrow_mut().entry(name) { std::collections::hash_map::Entry::Occupied(hit) => *hit.get(), std::collections::hash_map::Entry::Vacant(slot) => { @@ -9045,10 +8999,7 @@ impl Scene { // version of this diagnostic report a constant. let visible_path: &str; let mut visible_candidates: usize; - // `visible` costs ~400 ms over 444 937 candidates, and two things in - // the loop could own that: resolving each handle through the document's - // hash index, or the predicate, which looks a layer up **by name** — - // a string hash per entity. Timed separately, under PERF only. + // PERF-only breakdown for index maintenance and handle resolution. let mut visible_index_ms = 0.0f64; let mut visible_probe_ms = 0.0f64; @@ -9118,19 +9069,9 @@ impl Scene { .filter(|e| visibility_ok(e)) .collect() } else { - // Ask the per-epoch membership index for this block's candidates - // instead of testing every entity in the document. For a paper - // sheet holding 43 entities in a 1.17 M-entity drawing that is the - // difference between 313 ms and nothing measurable. - // - // The index is filled by walking `document.entities()`, so each - // block's list is already in document order and the wires come out - // in exactly the order the full scan produced. + // The per-epoch index preserves document order for each block. visible_path = "block-index"; - // The index itself: rebuilt by walking the whole document once per - // `geometry_epoch`, and the epoch moves on every edit. Timed - // separately because it is inside `visible` and is not per-entity - // work, so no per-candidate breakdown can ever account for it. + // Time index maintenance separately from the candidate loop. let t_members = perf.then(iced::time::Instant::now); let members = self.block_members(); visible_index_ms = crate::perf::elapsed_ms(t_members); @@ -9146,13 +9087,7 @@ impl Scene { } } if perf { - // Resolve-only pass, for the share of `visible` that is the - // document's hash index rather than the predicate. Repeated - // rather than clocked per iteration, and PERF only. - // - // It runs after the real loop, so it reads a warm cache and - // undercounts what the first traversal actually paid. Treat it - // as a floor, not as the cost. + // PERF-only lower bound for warm handle resolution. let t_probe = iced::time::Instant::now(); let mut resolved = 0usize; for &handle in claimed { @@ -9463,18 +9398,8 @@ vis_index={:.1} visible_probe={:.1}", std::cell::Ref::map(self.unindexable_cache.borrow(), |c| c.as_ref().unwrap()) } - /// Resolve owner handles and block-record membership once per geometry epoch. - /// - /// The walk visits every entity in the drawing and the epoch moves on every - /// edit, so it used to run again for each one — 19 ms of a 55 ms selection - /// on a 586 k-entity drawing, to add a single handle to a single list. - /// - /// A lone addition is folded in instead. Only a lone one: entities are - /// appended to the document, so a single new handle belongs at the end of - /// its block's list, while `replay_since` returns several changes in no - /// particular order and each list is in document order because that is - /// draw order. Anything else — a removal, a modification that could move an - /// entity between blocks, more than one change — takes the full walk. + /// Resolve block membership once per geometry epoch. A single addition is + /// appended safely; any other delta rebuilds the document-ordered index. fn block_members(&self) -> std::cell::Ref<'_, BlockMembers> { let mut cached_epoch = None; { @@ -10691,12 +10616,7 @@ mod journal_tests { // Differential oracle: the incrementally-patched entity index must always // equal a from-scratch rebuild after any add / move / erase. - #[test] - /// The partition skip turns on a membership test, and the test has to tell - /// a drawn line from a drawn circle. A line carries a tangent geom for - /// snapping and no triangles, so the cheap structural filter admits it — - /// deciding on that alone called every line a contributor and the skip - /// never fired once in a real session. + /// A line has tangent geometry but does not feed the analytical uploads. #[test] fn the_partition_membership_test_separates_lines_from_curves() { use acadrust::entities::{Circle, Line}; @@ -10730,10 +10650,7 @@ mod journal_tests { ); } - /// The partition skip reuses uploads that bake a draw depth, and it is only - /// sound while existing depths hold still. An incremental add must leave - /// them alone and must not bump the generation; a rebuild from scratch may - /// move them and must bump it. + /// Incremental additions preserve existing depths; full rebuilds invalidate them. #[test] fn adding_an_entity_leaves_existing_draw_depths_alone() { use acadrust::entities::Line; @@ -10776,9 +10693,7 @@ mod journal_tests { ); } - /// Folding a lone addition must land the handle where the full walk would - /// have put it — at the end of its block's list, because that list is in - /// document order and document order is draw order. + /// A folded addition must match the full document-order scan. #[test] fn folding_one_addition_matches_the_full_scan() { use acadrust::entities::Line; @@ -10863,13 +10778,8 @@ mod journal_tests { assert_eq!(indexed(&s, block), scanned(&s, block), "after modify"); } - /// The layer memo keys on the entity's **raw** layer string, while - /// `Table::get` normalises with `to_uppercase`. Two spellings of one layer - /// therefore take two memo slots, and both must resolve to that same layer - /// — otherwise the memo would hide geometry the unmemoized path shows. - /// - /// Covers the property the memo rests on (the table is case-insensitive) - /// and the predicate that reads it, not the memo's own bookkeeping. + /// Raw layer spellings used as memo keys must resolve through the + /// case-insensitive table. #[test] fn the_layer_memo_respects_case_insensitive_layer_names() { use acadrust::entities::Line; @@ -10889,8 +10799,7 @@ mod journal_tests { line.common.layer = layer.to_string(); scene.add_entity(EntityType::Line(line)) }; - // Three spellings of the same, switched-off layer, plus one that is - // genuinely a different layer and stays visible. + // Spellings of the switched-off layer must all remain hidden. on_layer("Walls"); on_layer("WALLS"); on_layer("walls"); @@ -10909,7 +10818,7 @@ mod journal_tests { "every spelling of a switched-off layer must be hidden", ); - // The memo takes one slot per spelling; each must find the same layer. + // Each spelling resolves to the same table entry. let resolved: Vec<_> = ["Walls", "WALLS", "walls"] .iter() .map(|name| scene.document.layers.get(name).map(|l| l.name.clone())) @@ -10922,10 +10831,7 @@ mod journal_tests { ); } - /// The loader computes the draw-order map and the receiving scene used to - /// compute it again — a walk of every entity, ~300 ms, for a map that - /// already existed. Handing it over is only sound if it describes the - /// receiving scene's document exactly, so that is what this asserts. + /// The loader's handed-over draw depths must equal a local rebuild. #[test] fn the_handed_over_draw_depth_map_matches_a_recompute() { use acadrust::entities::{Circle, Line}; @@ -10942,7 +10848,7 @@ mod journal_tests { Vector3::new(3.0, 0.0, 0.0), ))); - // What the loader would hand over, and what it built it from. + // Capture the map the loader would hand over. let expected = scene.draw_depth_map(); let handed_over = scene .draw_depth_cache @@ -10950,8 +10856,7 @@ mod journal_tests { .take() .expect("the map was just computed, so it is cached"); - // Installing it stands in for `install_prepared_open_geometry`: the - // epoch is the receiving scene's, the contents are the loader's. + // Install it under the receiving scene's epoch. scene.bump_geometry(); scene.install_prepared_open_geometry(PreparedOpenGeometry { wires: Arc::new(Vec::new()), @@ -10967,7 +10872,7 @@ mod journal_tests { "the handed-over map must be served as-is", ); - // ...and it must be the map this scene would have produced itself. + // A local rebuild must agree. *scene.draw_depth_cache.borrow_mut() = None; assert_eq!( *scene.draw_depth_map(), diff --git a/src/scene/pipeline/mod.rs b/src/scene/pipeline/mod.rs index 9b3b45b99..74313aa61 100644 --- a/src/scene/pipeline/mod.rs +++ b/src/scene/pipeline/mod.rs @@ -251,9 +251,7 @@ pub struct Pipeline { pub(crate) wire_arena_fallback_handles: rustc_hash::FxHashSet, /// The Model content id both arenas currently mirror (`u64::MAX` = none). pub(crate) wire_arena_id: u64, - /// Entities that fed the instanced / circle / ellipse uploads this slot is - /// holding. A patch touching none of them cannot change those uploads, so - /// the partition walk over the whole resident set can be skipped. + /// Handles that contributed to this slot's retained analytical uploads. pub(crate) partition_contributors: rustc_hash::FxHashSet, /// The draw-depth generation those uploads baked. A full depth rebuild /// reassigns every label, so they stop being reusable when it moves. @@ -4961,12 +4959,7 @@ impl Pipeline { } } - /// Forget every cache key that describes this slot's uploaded content. - /// - /// Shared by slot reuse and by [`Self::release_heavy_resources`] so the two - /// cannot disagree about what "this slot holds nothing you can trust" - /// means. Keys only — the buffers themselves are dropped by the caller that - /// wants the memory back. + /// Reset every cache key that describes this slot's uploaded content. pub(crate) fn forget_cached_keys(&mut self) { self.cached_epoch = (u64::MAX, u64::MAX, u64::MAX); self.cached_wire_id = u64::MAX; @@ -4989,10 +4982,7 @@ impl Pipeline { self.silhouette_key = (usize::MAX, u64::MAX, [u32::MAX; 3], false); self.silhouette_source_key = (usize::MAX, usize::MAX, u64::MAX); self.render_sig = u64::MAX; - // The partition record describes uploads this slot is no longer - // entitled to reuse. A reused slot rebuilds anyway, because its content - // id changed, but leaving a stale record here would make that an - // accident rather than a guarantee. + // The retained-upload record is part of the slot's cached state. self.partition_contributors.clear(); self.partition_depth_generation = u64::MAX; } diff --git a/src/scene/pipeline/wire_arena.rs b/src/scene/pipeline/wire_arena.rs index 322dea9f0..265ef257d 100644 --- a/src/scene/pipeline/wire_arena.rs +++ b/src/scene/pipeline/wire_arena.rs @@ -174,33 +174,16 @@ pub struct PartitionedWires<'a> { pub instanced: Vec<&'a WireModel>, pub circle_instances: Vec, pub ellipse_instances: Vec, - /// Entities that fed `instanced` / `circle_instances` / `ellipse_instances`. - /// - /// A patch that touches none of these cannot change those three uploads, so - /// the caller can keep the ones it already has and skip the walk entirely. + /// Handles that contributed to the retained analytical uploads. pub contributors: rustc_hash::FxHashSet, } -/// Whether a change to this wire can alter what `partition_wires` produces for -/// the instanced, circle and ellipse uploads. -/// -/// Decided by attempting the extraction, exactly as the walk decides it. The -/// cheap structural test alone is not enough: an ordinary drawn line carries a -/// tangent geom for snapping and no triangles, so it passes that test, fails -/// extraction, and lands in `regular` — a membership test that stopped there -/// would call every line a contributor and never skip anything. -/// -/// `draw_depth` only rides along in the extracted instance, so the value passed -/// here cannot change the answer. +/// Whether this wire contributes to an upload retained across arena patches. pub fn feeds_analytical_uploads(wire: &WireModel) -> bool { - if !wire.display_visible { - return false; - } - if wire.render_instance.is_some() { - return true; - } - super::circle_gpu::extract_circle_instances(wire, 0.0).is_some() - || super::ellipse_gpu::extract_ellipse_instances(wire, 0.0).is_some() + matches!( + classify_wire(wire), + WireKind::Block | WireKind::Circle | WireKind::Ellipse + ) } /// Single-pass classification and extraction of all viewport wire categories. @@ -233,10 +216,7 @@ pub fn partition_wires<'a>( instanced.push(wire); continue; } - // `wire_draw_depth` is a hash lookup and its result is used only by the - // analytical extractions below, which most wires never reach. Computing - // it first cost one lookup per resident wire on every patch — 236 956 - // of them for the ~56 000 that are circles or ellipses. + // Resolve draw depth only for wires eligible for analytical extraction. if !wire.tangent_geoms.is_empty() && wire.fill_tris.is_empty() && wire.pick_tris.is_empty() diff --git a/src/scene/selection.rs b/src/scene/selection.rs index de9248c31..d77e6d88a 100644 --- a/src/scene/selection.rs +++ b/src/scene/selection.rs @@ -360,21 +360,8 @@ impl Scene { self.selected.len() } - /// Sorted entity types in the current layout, cached until geometry or layout changes. - /// The entity type names present in the current layout, for the selection - /// filter menu. - /// - /// The status bar asks for this on **every** frame, and the cache used to - /// be keyed on `geometry_epoch` alone — so every drawn point invalidated it - /// and the walk below visited all 444 937 handles a model space owns. That - /// was ~100 ms per edit, the largest single cost of drawing a line, and - /// almost always to arrive at the same answer: adding a line to a drawing - /// that already has lines changes nothing here. - /// - /// An edit is now folded in from the delta journal instead. Additions only - /// ever grow the set, so they are cheap to apply; a removal can retire a - /// type and cannot be resolved from a handle that is already gone, so those - /// fall back to the full walk. + /// Entity type names in the current layout for the selection-filter menu. + /// Pure additions are folded into the cached set; other changes rebuild it. pub fn entity_type_names_in_layout(&self) -> std::sync::Arc> { use crate::entities::traits::entity_type_name; let block = self.current_layout_block_handle(); @@ -394,7 +381,10 @@ impl Scene { // Incremental: fold the changes since the cached epoch into the set. if let Some(since) = cached_epoch { if let Some(deltas) = self.replay_since(since) { - if !deltas.iter().any(|(_, kind)| *kind == ChangeKind::Removed) { + if deltas + .iter() + .all(|(_, kind)| *kind == ChangeKind::Added) + { let mut cache = self.layout_type_names_cache.borrow_mut(); if let Some((epoch, _, present, names)) = cache.as_mut() { let mut added = false; @@ -1013,10 +1003,6 @@ impl Scene { mod tests { use super::*; - /// The reason the cache carries a set as well as a list: drawing into a - /// drawing that already has that type must not rebuild either, because the - /// rebuild walks every entity the layout owns — 444 937 of them on the - /// reproducer, ~100 ms, on every single drawn point. #[test] fn adding_a_type_already_present_reuses_the_list() { use acadrust::entities::{Circle, EntityType, Line}; @@ -1053,6 +1039,24 @@ mod tests { ); } + #[test] + fn changing_an_entity_type_rebuilds_the_type_names() { + use acadrust::entities::{Circle, EntityType, Line}; + use acadrust::types::Vector3; + + let mut scene = Scene::new(); + let handle = scene.add_entity(EntityType::Line(Line::from_points( + Vector3::new(0.0, 0.0, 0.0), + Vector3::new(1.0, 0.0, 0.0), + ))); + assert_eq!(scene.entity_type_names_in_layout().as_slice(), ["Line"]); + + let mut circle = Circle::new(); + circle.common.handle = handle; + assert!(scene.update_entity(EntityType::Circle(circle))); + assert_eq!(scene.entity_type_names_in_layout().as_slice(), ["Circle"]); + } + #[test] fn layout_type_cache_reuses_and_invalidates_on_edits_undo_and_layout() { use acadrust::entities::{Circle, EntityType, Line}; diff --git a/src/scene/view/render.rs b/src/scene/view/render.rs index 026fa517b..331a536f5 100644 --- a/src/scene/view/render.rs +++ b/src/scene/view/render.rs @@ -842,18 +842,8 @@ impl shader::Primitive for Primitive { gpus.extend(arena.wire_gpus()); } inner.gpu_wires = std::sync::Arc::new(gpus); - // A one-entity patch costs 73 ms in this block, and the - // three steps below are all O(resident set) rather than - // O(changes). Which of them dominates decides the fix. - // The partition walks the whole resident set, and on - // a patch it almost always produces the three uploads it - // produced last frame. Skip it when no changed entity - // feeds them and none did before. - // - // Safe because adding an entity does not disturb any - // other entity's draw depth: `inserted_draw_depth_label` - // allocates into the reserved label gap rather than - // renumbering, so instances already uploaded stay right. + // Retain analytical uploads when a patch changes neither + // their contributors nor their baked draw depths. let analytical_untouched = _patched && inner.partition_depth_generation == vp.draw_depth_generation && patch.is_some_and(|patch| { @@ -910,10 +900,7 @@ impl shader::Primitive for Primitive { .map_or(0.0, |t| t.elapsed().as_secs_f64() * 1000.0); } if _perf { - // Counts only when a partition produced them. - // Printing zeros on a skip reads as "no circles" - // rather than "not recomputed", which is the - // opposite of what the line is reporting. + // Report counts only when partitioning ran. match &partitioned { Some(partitioned) => crate::perf_record!( "[perf] arena-post partition={part_ms:.1}ms \