Merge pull request #1076 from mechpanos/pr/skip-partition-on-patch

src/scene/mod.rs

@@ -1715,6 +1715,12 @@ 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<Option<DrawDepthCache>>,
+    /// 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.
+    draw_depth_generation: std::cell::Cell<u64>,
     /// Shared empty map for 3-D wireframe, where true depth wins instead of
     /// the entity submission order used by the optimized 2-D style.
     no_draw_depths: Arc<HashMap<u64, [f32; 2]>>,
@@ -2075,6 +2081,7 @@ impl Scene {
             interaction_handle_index_cache: RefCell::new(None),
             sort_cache: RefCell::new(None),
             draw_depth_cache: RefCell::new(None),
+            draw_depth_generation: std::cell::Cell::new(0),
             no_draw_depths: Arc::new(HashMap::default()),
             hatch_cache: RefCell::new(HashMap::default()),
             wipeout_cache: RefCell::new(HashMap::default()),
@@ -6030,6 +6037,11 @@ impl Scene {
     /// A full build assigns sparse labels in effective draw order. Incremental
     /// Add/Remove then changes only the named handle: existing siblings retain
     /// their depth, avoiding an O(all entities) map rewrite and GPU const upload.
+    /// See [`Self::draw_depth_generation`].
+    pub(in crate::scene) fn draw_depth_generation(&self) -> u64 {
+        self.draw_depth_generation.get()
+    }
+
     pub(super) fn draw_depth_map(&self) -> Arc<HashMap<u64, [f32; 2]>> {
         {
             let cache = self.draw_depth_cache.borrow();
@@ -6159,6 +6171,11 @@ impl Scene {
             }
         }
 
+        // From here the labels are assigned afresh, so an existing entity's
+        // depth can move. Anything holding a baked depth has to rebuild.
+        self.draw_depth_generation
+            .set(self.draw_depth_generation.get().wrapping_add(1));
+
         use acadrust::objects::ObjectType;
         // Per-block SortEntitiesTable overrides: block -> (entity_val -> sort_val).
         let mut overrides: HashMap<Handle, HashMap<u64, u64>> = HashMap::default();
@@ -10675,6 +10692,90 @@ 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.
+    #[test]
+    fn the_partition_membership_test_separates_lines_from_curves() {
+        use acadrust::entities::{Circle, Line};
+        use acadrust::types::Vector3;
+        use crate::scene::pipeline::wire_arena::feeds_analytical_uploads;
+
+        let mut scene = Scene::new();
+        let feeds = |scene: &Scene, handle: Handle| {
+            let entity = scene.document.get_entity(handle).unwrap().clone();
+            scene
+                .tessellate_one(&entity)
+                .iter()
+                .any(feeds_analytical_uploads)
+        };
+
+        let line = scene.add_entity(EntityType::Line(Line::from_points(
+            Vector3::new(0.0, 0.0, 0.0),
+            Vector3::new(10.0, 5.0, 0.0),
+        )));
+        assert!(
+            !feeds(&scene, line),
+            "a drawn line must not force the partition walk",
+        );
+
+        let mut circle = Circle::new();
+        circle.radius = 4.0;
+        let circle = scene.add_entity(EntityType::Circle(circle));
+        assert!(
+            feeds(&scene, circle),
+            "a drawn circle feeds the analytical uploads and must force the walk",
+        );
+    }
+
+    /// 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.
+    #[test]
+    fn adding_an_entity_leaves_existing_draw_depths_alone() {
+        use acadrust::entities::Line;
+        use acadrust::types::Vector3;
+
+        fn add(scene: &mut Scene, x: f64) -> Handle {
+            scene.add_entity(EntityType::Line(Line::from_points(
+                Vector3::new(x, 0.0, 0.0),
+                Vector3::new(x + 1.0, 0.0, 0.0),
+            )))
+        }
+        let mut scene = Scene::new();
+        let first = add(&mut scene, 0.0);
+        let second = add(&mut scene, 1.0);
+        let before = scene.draw_depth_map();
+        let generation = scene.draw_depth_generation();
+
+        add(&mut scene, 2.0);
+        let after = scene.draw_depth_map();
+        assert_eq!(
+            scene.draw_depth_generation(),
+            generation,
+            "an incremental add must not invalidate baked depths",
+        );
+        for handle in [first, second] {
+            assert_eq!(
+                after.get(&handle.value()),
+                before.get(&handle.value()),
+                "an existing entity's depth must not move when another is added",
+            );
+        }
+
+        // A rebuild from scratch reassigns labels, so it has to say so.
+        scene.draw_depth_cache.borrow_mut().take();
+        let _ = scene.draw_depth_map();
+        assert_ne!(
+            scene.draw_depth_generation(),
+            generation,
+            "a full rebuild must invalidate anything holding a baked depth",
+        );
+    }
+
     /// 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.

src/scene/pipeline/mod.rs

@@ -251,6 +251,13 @@ pub struct Pipeline {
     pub(crate) wire_arena_fallback_handles: rustc_hash::FxHashSet<acadrust::Handle>,
     /// 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.
+    pub(crate) partition_contributors: rustc_hash::FxHashSet<acadrust::Handle>,
+    /// The draw-depth generation those uploads baked. A full depth rebuild
+    /// reassigns every label, so they stop being reusable when it moves.
+    pub(crate) partition_depth_generation: u64,
     /// Last content/camera/viewport tuple used to derive visible instance
     /// ranges from the resident arena.
     pub(crate) wire_cull_key: (u64, u64, u32, u32),
@@ -2338,6 +2345,8 @@ impl Pipeline {
             wire_arena_fallback_kind: None,
             wire_arena_fallback_handles: rustc_hash::FxHashSet::default(),
             wire_arena_id: u64::MAX,
+            partition_contributors: rustc_hash::FxHashSet::default(),
+            partition_depth_generation: u64::MAX,
             wire_cull_key: (u64::MAX, u64::MAX, 0, 0),
             hatch_lod_key: (usize::MAX, u64::MAX, 0, 0, false),
             wipeout_lod_key: (usize::MAX, u64::MAX, 0, 0, false),
@@ -4980,6 +4989,12 @@ 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.
+        self.partition_contributors.clear();
+        self.partition_depth_generation = u64::MAX;
     }
 
     /// Catch errors delivered after the previous upload/submit completed.
@@ -4993,6 +5008,8 @@ impl Pipeline {
         self.wire_arena = None;
         self.wire_arena_mesh = None;
         self.wire_arena_id = u64::MAX;
+        self.partition_contributors.clear();
+        self.partition_depth_generation = u64::MAX;
         self.alloc_size = Size::new(0, 0);
         self.shadow_full = None;
         self.background_source_id = usize::MAX;
@@ -5024,6 +5041,8 @@ impl Pipeline {
         self.wire_arena_fallback_kind = None;
         self.wire_arena_fallback_handles.clear();
         self.wire_arena_id = u64::MAX;
+        self.partition_contributors.clear();
+        self.partition_depth_generation = u64::MAX;
 
         self.gpu_selected_wires.clear();
         self.gpu_selected_block_wires.clear();

src/scene/pipeline/wire_arena.rs

@@ -174,6 +174,33 @@ pub struct PartitionedWires<'a> {
     pub instanced: Vec<&'a WireModel>,
     pub circle_instances: Vec<super::circle_gpu::CircleInstance>,
     pub ellipse_instances: Vec<super::ellipse_gpu::EllipseInstance>,
+    /// 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.
+    pub contributors: rustc_hash::FxHashSet<Handle>,
+}
+
+/// 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.
+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()
 }
 
 /// Single-pass classification and extraction of all viewport wire categories.
@@ -190,26 +217,39 @@ pub fn partition_wires<'a>(
     let mut instanced = Vec::new();
     let mut circle_instances = Vec::new();
     let mut ellipse_instances = Vec::new();
+    let mut contributors: rustc_hash::FxHashSet<Handle> = rustc_hash::FxHashSet::default();
+    let note = |wire: &WireModel, set: &mut rustc_hash::FxHashSet<Handle>| {
+        if let Some(handle) = handle_of(wire) {
+            set.insert(handle);
+        }
+    };
 
     for wire in wires {
         if !wire.display_visible {
             continue;
         }
         if wire.render_instance.is_some() {
+            note(wire, &mut contributors);
             instanced.push(wire);
             continue;
         }
-        let depth = super::wire_gpu::wire_draw_depth(wire, depth_map);
+        // `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.
         if !wire.tangent_geoms.is_empty()
             && wire.fill_tris.is_empty()
             && wire.pick_tris.is_empty()
             && wire.text_verts.is_empty()
         {
+            let depth = super::wire_gpu::wire_draw_depth(wire, depth_map);
             if let Some(insts) = super::circle_gpu::extract_circle_instances(wire, depth) {
+                note(wire, &mut contributors);
                 circle_instances.extend(insts);
                 continue;
             }
             if let Some(insts) = super::ellipse_gpu::extract_ellipse_instances(wire, depth) {
+                note(wire, &mut contributors);
                 ellipse_instances.extend(insts);
                 continue;
             }
@@ -229,6 +269,7 @@ pub fn partition_wires<'a>(
         instanced,
         circle_instances,
         ellipse_instances,
+        contributors,
     }
 }
 

src/scene/view/render.rs

@@ -149,6 +149,9 @@ pub struct ViewportData {
     /// by the wire / face3d pipelines as a clip-z bias. WireModels carry no
     /// depth field (84 construction sites); the bias is looked up by handle
     /// at GPU-upload time from this map instead.
+    /// See `Scene::draw_depth_generation`. Cached uploads that bake a depth are
+    /// only reusable while this is unchanged.
+    pub(in crate::scene) draw_depth_generation: u64,
     pub(in crate::scene) draw_depths:
         std::sync::Weak<rustc_hash::FxHashMap<u64, [f32; 2]>>,
     pub(in crate::scene) hatches: Arc<Vec<HatchModel>>,
@@ -839,31 +842,96 @@ impl shader::Primitive for Primitive {
                             gpus.extend(arena.wire_gpus());
                         }
                         inner.gpu_wires = std::sync::Arc::new(gpus);
-                        let partitioned =
-                            wire_arena::partition_wires(&vp_wires, &draw_depths);
-                        inner.gpu_block_wires = std::sync::Arc::new(
-                            inner.upload_block_wires(
-                                device,
-                                queue,
-                                &partitioned.instanced,
-                                &draw_depths,
-                                &mut pipeline.block_geometry,
-                            ),
-                        );
-                        inner.gpu_circles = std::sync::Arc::new(
-                            inner.upload_circles_from_instances(
-                                device,
-                                queue,
-                                &partitioned.circle_instances,
-                            ),
-                        );
-                        inner.gpu_ellipses = std::sync::Arc::new(
-                            inner.upload_ellipses_from_instances(
-                                device,
-                                queue,
-                                &partitioned.ellipse_instances,
-                            ),
-                        );
+                        // 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.
+                        let analytical_untouched = _patched
+                            && inner.partition_depth_generation == vp.draw_depth_generation
+                            && patch.is_some_and(|patch| {
+                                patch.changes.iter().all(|(handle, _)| {
+                                    !inner.partition_contributors.contains(handle)
+                                        && !patch.runs.get(handle).is_some_and(|run| {
+                                            run.iter()
+                                                .any(wire_arena::feeds_analytical_uploads)
+                                        })
+                                })
+                            });
+                        let t_part = _perf.then(iced::time::Instant::now);
+                        let partitioned = (!analytical_untouched)
+                            .then(|| wire_arena::partition_wires(&vp_wires, &draw_depths));
+                        let part_ms = t_part
+                            .map_or(0.0, |t| t.elapsed().as_secs_f64() * 1000.0);
+                        let t_blk = _perf.then(iced::time::Instant::now);
+                        let mut blk_ms = 0.0f64;
+                        let mut curve_ms = 0.0f64;
+                        // `None` means the three uploads this slot holds are
+                        // still the answer, so they are left alone.
+                        if let Some(partitioned) = &partitioned {
+                            inner.gpu_block_wires = std::sync::Arc::new(
+                                inner.upload_block_wires(
+                                    device,
+                                    queue,
+                                    &partitioned.instanced,
+                                    &draw_depths,
+                                    &mut pipeline.block_geometry,
+                                ),
+                            );
+                            blk_ms = t_blk
+                                .map_or(0.0, |t| t.elapsed().as_secs_f64() * 1000.0);
+                            let t_curve = _perf.then(iced::time::Instant::now);
+                            inner.gpu_circles = std::sync::Arc::new(
+                                inner.upload_circles_from_instances(
+                                    device,
+                                    queue,
+                                    &partitioned.circle_instances,
+                                ),
+                            );
+                            inner.gpu_ellipses = std::sync::Arc::new(
+                                inner.upload_ellipses_from_instances(
+                                    device,
+                                    queue,
+                                    &partitioned.ellipse_instances,
+                                ),
+                            );
+                            inner
+                                .partition_contributors
+                                .clone_from(&partitioned.contributors);
+                            inner.partition_depth_generation = vp.draw_depth_generation;
+                            curve_ms = t_curve
+                                .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.
+                            match &partitioned {
+                                Some(partitioned) => crate::perf_record!(
+                                    "[perf] arena-post partition={part_ms:.1}ms \
+blocks={blk_ms:.1}ms curves={curve_ms:.1}ms skipped=false wires={} instanced={} \
+circles={} ellipses={}",
+                                    vp_wires.len(),
+                                    partitioned.instanced.len(),
+                                    partitioned.circle_instances.len(),
+                                    partitioned.ellipse_instances.len(),
+                                ),
+                                None => crate::perf_record!(
+                                    "[perf] arena-post skipped=true wires={} \
+retained_contributors={}",
+                                    vp_wires.len(),
+                                    inner.partition_contributors.len(),
+                                ),
+                            }
+                        }
                         if _patched {
                             wire_arena::patch_handle_index(
                                 &mut inner.wire_handle_index,
@@ -4239,6 +4307,7 @@ impl Scene {
             face3d_wires,
             text_verts,
             preview_text_verts,
+            draw_depth_generation: self.draw_depth_generation(),
             draw_depths: Arc::downgrade(&draw_depths),
             hatches,
             wipeout_hatches,