8af41b3310
ARCHITECTURE.md invariant 2 has said since it was written that the scattered clear_mesh_cache() calls are "unnecessary and expensive", because MeshCache keys on (NodeId, ParamHash) and an edited node invalidates itself. Phase 5.2 removed the first one. This removes the rest: 8 of the 11 live call sites, leaving only invalidate_all (whose contract is exactly that) and the definition. Investigating them turned up something the invariant did not say, and which makes the cleanup a bug fix rather than tidying: MeshCache has NO eviction for deleted nodes. Entries are keyed by NodeId and nothing ever removes one when its node goes away. The wholesale clears were, accidentally, the cache's only garbage collector. Deleting them naively would have converted a performance problem into an unbounded memory leak over a session. So the fix is a targeted replacement, not a deletion: MeshCache::retain, SceneCache::retain_meshes and CadViewport::evict_dead_meshes drop exactly the entries whose node is gone and leave every live one warm. Each call site was then classified rather than pattern-matched: - deletes (delete_selected, delete_part, CommandContext::delete_node) -> evict dead entries - pure additions (create_node, insert_node_at, paste, and the wall / circle / rect / area / column / beam / polygon / polyline tool completions) -> nothing at all. A new node has no cache entry to invalidate and cannot affect any other node's mesh. These were discarding the entire scene's meshes to make room for nothing. - subdivide_selected -> invalidate only the selected ids. It builds a fresh Arc<Solid> per part, so ParamHash already differs; the unselected parts were being thrown away for no reason. Measured: deleting 1 of 100 parts costs 62.1 us with clear+rebuild versus 21.9 us with evict+reuse, a 3x saving on every delete. Two tests, both negative-tested by stubbing out the retain body: retain_nodes_evicts_dead_entries_and_keeps_live_ones checks both directions (dead gone, live still an Arc::ptr_eq hit), and cache_does_not_grow_past_the_live_node_set simulates 50 add/delete rounds and asserts the cache holds 1 entry, not 50. With retain stubbed it holds exactly 50, which is the leak. Also documented a sharp edge found while reading ParamHash: it hashes a Csg node's Arc POINTER, not its geometry. Re-wrapping identical geometry in a new Arc is therefore a safe false-positive, but mutating a Solid behind a shared Arc would go unnoticed. Nothing does that today and ARCHITECTURE.md now says it must stay that way. 624 lib + 154 integration, 0 failed.
6.6 KiB
CAD benchmark baseline
Reproduce with:
cargo test --locked --release -p nigig-build --lib -- \
--ignored --nocapture --test-threads=1 profile_benchmarks
Release mode matters. Debug builds are 3–5× slower here and the ratios between arms shift, so a debug number is not comparable to anything below.
These are measurements, not assertions — the benchmarks are #[ignore]-d
and deliberately contain no timing assert!s (a wall-clock assertion in
the normal test run flakes under CI load). Compare against this file by
hand when changing the hot paths.
Recorded on the Phase 3 tree, x86-64, --release.
| Benchmark | Result |
|---|---|
bench_scene_cache_hit_vs_rebuild |
cold 25.9 µs / warm 53 ns (100 parts) — 488× |
bench_scene_cache_scaling |
10/50/100/500 parts: cold scales linearly, warm stays ~150 ns |
bench_mesh_cache_hit_cost |
18.7 µs/frame, 186 ns per part (100 parts, all hits) |
bench_size_parametric_vs_mesh_derived |
parametric 2 ns vs mesh-derived 886 ns — 443× |
bench_pick_broadphase_mesh_bounds_vs_size |
63.3 µs vs 131.0 µs per frame — 2.1× saved |
bench_glb_export_with_and_without_cache |
cold 1.42 ms / warm 1.35 ms (50 parts) |
bench_parallel_vs_sequential_export |
sequential 4.83 ms / parallel 3.58 ms (100 parts) — 1.3× |
bench_command_execute_overhead |
0.4 µs per execute, 0.4 µs per undo |
bench_gpu_upload_mesh_source |
boxes: rebuild 0.27 µs/part vs warm cache 0.178 µs — 2×; extruded 24-gons: 1.11 µs vs 0.516 µs — 2×. Cold cache 0.79 µs/part is slower than rebuilding (hash + insert) |
bench_delete_invalidation_clear_vs_evict |
deleting 1 of 100 parts: clear+rebuild 62.1 µs vs evict+reuse 21.9 µs — 3× |
bench_viewport_snapshot_sync_per_frame |
deep copies 21.2 µs/frame + forced rebuilds 77.2 µs/frame = 98.5 µs/frame; generation-checked 0.051 µs — 1930× (100 parts) |
Note on bench_gpu_upload_mesh_source
Phase 5.3 routed the GPU upload path through MeshCache instead of
calling build_solid() directly. The measured speed win is 2× on a warm
cache and negative on a cold one — a first-ever build now pays a
ParamHash and a map insert on top of the meshing.
That trade is worth taking, but not primarily for speed:
ensure_part_geometryonly builds for parts with no uploaded buffer, so the cold case is a scene load or an undo/redo, not a frame in a loop.- The warm case is every part that was already exported, hovered or previously uploaded — which, during editing, is most of them.
- The real gain is that there is now one node-to-triangles path. Two
independent
matcharms over the same enum were free to drift, and nothing would have caught it: the preview would simply have disagreed with the export.
Do not "optimise" this back by special-casing cheap primitives. The 0.5 µs is not worth reintroducing the second pipeline.
Note on bench_viewport_snapshot_sync_per_frame
This benchmark measures the primitives the sync path is built from --
Vec<CadNode> clones and SceneCache::scene() rebuilds -- not the widget
method itself, which needs a live Cx and three real viewports. Its
printed numbers therefore do not move when the sync path is fixed;
they quantify what one frame of syncing costs and what the same frame
costs when a generation check is allowed to short-circuit it.
Read it as the size of the prize, not as a regression gate. The Phase 5.2
change is verified by behaviour tests
(replace_all_bumps_the_generation_so_destinations_can_compare,
mesh_cache_self_invalidates_on_parameter_and_transform_edits,
geometry_is_retained_for_surviving_ids_only), each negative-tested.
Reading these
The scene cache is doing its job. 488× on a warm hit, and the scaling run shows the warm path is O(1) while the cold path is linear. No work needed here.
MeshCache hits are not free. 186 ns per part is ParamHash::from_node
being recomputed on every lookup — it hashes the solid's every field, the
transform and the material, even when the entry is present and valid. At
100 parts that is ~19 µs per hover frame. Not the dominant cost today, but
it is the next thing to fix if picking needs to get faster: memoise the
hash on CadNode and invalidate it on mutation.
CadNode::size() is a trap. 2 ns for a parametric solid, 886 ns for a
CSG or extruded one, because those have no closed-form extent and the
function meshes the solid to derive a bounding box (added in Phase 1.7 to
fix unpickable parts). It reads like a cheap field access at the call site.
Anything calling it in a loop should take bounds from a cached mesh
instead — which is exactly what pick_part now does.
Export is already fast enough. 1.4 ms for 50 parts, and the mesh cache barely moves it because JSON/binary serialisation dominates, not meshing. The parallel path wins only 1.3× at 100 parts. Neither is worth tuning before the UI hot paths.
What Phase 3 changed
| Change | Effect |
|---|---|
pick_part takes AABB bounds from the mesh it already holds instead of calling size() |
2.1× less broad-phase work per hover frame; also more correct — see below |
Hover picking throttled by HOVER_PICK_MIN_MOVE_PX (3 px) |
Sub-pixel jitter no longer triggers a full ray cast against every triangle |
Removed cx.redraw_all() from the part-drag MouseMove handler |
No full-widget-tree relayout per motion event during a drag |
The AABB change also fixed a latent correctness bug. size() returns a
symmetric extent about the origin, but an extruded polygon grows along
+Y from its base plane — so the old broad-phase box was in the wrong
place and could reject a ray that actually hits. pick_bounds_tests:: mesh_bounds_are_asymmetric_where_size_is_not pins this.
Not done, and why
Memoising ParamHash on CadNode (~186 ns/part/frame). It needs a
cache field on the node plus invalidation on every mutation path, which is
the same ownership problem Phase 5.1 solves properly with a generation
counter. Doing it now would add a second thing to keep in sync by hand —
exactly the pattern this codebase already suffers from.
A BVH or spatial index for picking. Current cost is linear in part count with a cheap early-out. At the 10–100 parts these models actually contain, a BVH would cost more to build than it saves. Revisit if scenes grow an order of magnitude.
The other 81 cx.redraw_all() calls. Most are on discrete user
actions (keypress, button, tool change) where a full redraw is once-per-
gesture and harmless. Only the per-motion ones were worth removing, and
orbit_3d_by_screen_delta / pan_3d were already correct.