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.
131 lines
6.6 KiB
Markdown
131 lines
6.6 KiB
Markdown
# CAD benchmark baseline
|
||
|
||
Reproduce with:
|
||
|
||
```bash
|
||
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_geometry` only 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 `match` arms 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.
|