forked from 1b-agent/opencadstudio-upstream
perf: skip the wire partition when a patch cannot change its output
`arena-post partition=55.2ms blocks=5.9ms curves=2.5ms` — the partition is almost the whole of the 73 ms an arena patch spends after patching, and it walks all 236 956 resident wires to serve a one-entity change. Its three consumers here — the instanced, circle and ellipse uploads — depend only on wires that are block references or analytical curves. A patch that touches none of those cannot change any of them, so the walk is skipped and the uploads already on the slot are kept. Two things make that sound, and both are checked rather than assumed: **Depths hold still.** The instances bake a draw depth. `inserted_draw_depth_label` allocates into a reserved gap instead of renumbering, so adding an entity leaves every other label alone — the existing doc comment says so and there is now a test. A rebuild from scratch *can* move them, so `draw_depth_generation` bumps only there, and a slot reuses its uploads only while that generation is unchanged. **Contributors are remembered.** `partition_wires` records which handles fed those three outputs, and the skip requires that no changed handle is among them and that no changed handle's new wires would join them. That catches an entity entering the set and an entity leaving it. The membership test is shared with the walk it decides to skip, so the two cannot drift, and it is deliberately conservative: a wire that looks analytical but fails extraction forces a needless re-partition, never a stale upload. `arena-post` gains `skipped=` so a run says how often this actually fires rather than leaving it to be inferred from the timing. Not verified on a device yet — the headless harness never reaches `prepare`, so this needs a GUI run on the reproducer to confirm both the saving and that nothing goes missing on screen. Co-Authored-By: Claude Opus 5 <[email protected]>
This commit is contained in:
1 parent
b68989768d
commit
03c64e3d38
4 files changed
+176
-33
No files matched your search
@@ -1706,6 +1706,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]>>,
|
||||
@@ -2053,6 +2059,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()),
|
||||
@@ -5998,6 +6005,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();
|
||||
@@ -6127,6 +6139,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();
|
||||
@@ -10643,6 +10660,52 @@ 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 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.
|
||||
|
||||
@@ -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),
|
||||
@@ -4991,6 +5000,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;
|
||||
@@ -5022,6 +5033,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();
|
||||
|
||||
@@ -174,6 +174,27 @@ 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.
|
||||
///
|
||||
/// Deliberately conservative: a wire that looks analytical here may still fail
|
||||
/// extraction and end up in `regular`, which costs an unnecessary re-partition
|
||||
/// and never a stale upload. Shared with `partition_wires` so the test that
|
||||
/// decides to skip cannot drift from the walk it is skipping.
|
||||
pub fn feeds_analytical_uploads(wire: &WireModel) -> bool {
|
||||
wire.display_visible
|
||||
&& (wire.render_instance.is_some()
|
||||
|| (!wire.tangent_geoms.is_empty()
|
||||
&& wire.fill_tris.is_empty()
|
||||
&& wire.pick_tris.is_empty()
|
||||
&& wire.text_verts.is_empty()))
|
||||
}
|
||||
|
||||
/// Single-pass classification and extraction of all viewport wire categories.
|
||||
@@ -190,12 +211,19 @@ 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;
|
||||
}
|
||||
@@ -210,10 +238,12 @@ pub fn partition_wires<'a>(
|
||||
{
|
||||
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;
|
||||
}
|
||||
@@ -233,6 +263,7 @@ pub fn partition_wires<'a>(
|
||||
instanced,
|
||||
circle_instances,
|
||||
ellipse_instances,
|
||||
contributors,
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
+69
-33
@@ -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>>,
|
||||
@@ -842,47 +845,79 @@ impl shader::Primitive for Primitive {
|
||||
// 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 =
|
||||
wire_arena::partition_wires(&vp_wires, &draw_depths);
|
||||
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);
|
||||
inner.gpu_block_wires = std::sync::Arc::new(
|
||||
inner.upload_block_wires(
|
||||
device,
|
||||
queue,
|
||||
&partitioned.instanced,
|
||||
&draw_depths,
|
||||
&mut pipeline.block_geometry,
|
||||
),
|
||||
);
|
||||
let 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,
|
||||
),
|
||||
);
|
||||
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 {
|
||||
crate::perf_record!(
|
||||
"[perf] arena-post partition={part_ms:.1}ms blocks={blk_ms:.1}ms \
|
||||
curves={:.1}ms wires={} instanced={} circles={} ellipses={}",
|
||||
t_curve.map_or(0.0, |t| t.elapsed().as_secs_f64() * 1000.0),
|
||||
curves={curve_ms:.1}ms skipped={} wires={} instanced={} circles={} ellipses={}",
|
||||
partitioned.is_none(),
|
||||
vp_wires.len(),
|
||||
partitioned.instanced.len(),
|
||||
partitioned.circle_instances.len(),
|
||||
partitioned.ellipse_instances.len(),
|
||||
partitioned.as_ref().map_or(0, |p| p.instanced.len()),
|
||||
partitioned.as_ref().map_or(0, |p| p.circle_instances.len()),
|
||||
partitioned.as_ref().map_or(0, |p| p.ellipse_instances.len()),
|
||||
);
|
||||
}
|
||||
if _patched {
|
||||
@@ -4259,6 +4294,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,
|
||||
|
||||
Reference in new issue
Block a user