Repository navigation
perf(topology): speed up traced tab generation 3.5–5.6× - #692
Conversation
|
🧩 Preview deployed: https://puzzlewise.github.io/dev/ |
Consolidated PR ReviewPR SummaryPR #692 ( Review tier: full — 335 changed lines across 5 files after noise filtering, so all six dimension reviewers ran. Merge Readiness -- READY TO MERGE ✅The PR is strictly better than
1. Backward Compatibility -- SAFE ✅The compatibility surface is the geometry reproducibility contract (share links and seeds must regenerate identical piece paths); the PR preserves it, and the code-level API change is additive and internal. No significant issues found.
2. Code Quality -- GOOD ✅The port is small, well-named and faithful to bezier-js 6.1.4, and the new
3. Architecture -- GOOD ✅Dependency direction is clean (the port imports only types from bezier-js;
4. Security -- SAFE ✅Pure numeric geometry code with no new inputs, I/O, network, storage, serialization, dependencies or privilege boundaries. No significant issues found.
5. Observability -- SAFE ✅Nothing in the PR logs, catches or wraps errors, and neither did the library code it replaces; the topology math modules have no logging on the target branch and the only signals they emit (thrown
6. Performance -- GOOD ✅The ports remove the hot allocations and repeated
Overall Verdicts
Top RecommendationsNo
|
|
Responses to the consolidated review: 1. 2. Unreached 3. Add
🤖 Generated with Claude Code |
4a5842b to
fba8d36
Compare
Consolidated PR ReviewPR SummaryPR #692 ( Review tier: full — 337 changed lines across 5 files after noise filtering, so all six dimension reviewers ran. Merge Readiness -- READY TO MERGE ✅The PR is strictly better than
1. Backward Compatibility -- SAFE ✅The compatibility surface is the geometry reproducibility contract (seeds and share links must regenerate identical piece paths); the PR preserves it, and its one API change is additive and internal. No significant issues found.
2. Code Quality -- GOOD ✅The prior round's
3. Architecture -- GOOD ✅Dependency direction is clean (the port imports only types from bezier-js;
4. Security -- SAFE ✅Pure numeric geometry with no new inputs, I/O, network, storage, serialization, dependencies or privilege boundaries. No significant issues found.
5. Observability -- SAFE ✅Nothing in the PR logs, catches or wraps errors, and neither did the library code it replaces; the topology math modules have no logging on the target branch. No significant issues found.
6. Performance -- GOOD ✅The port goes straight at the profiled hotspot: it drops the ~4 throwaway
Overall Verdicts
Top RecommendationsNo
|
Traced cut styles spent 60-68% of generation in bezier-js's reduce(): for every 0.01 step it builds several Bezier objects from a trial split only to ask simple() of the result. Tab candidates are fresh curves, so the per-curve reduction cache never hits. Port reduce() for 2D cubics and run the trial simple() check on raw control points with the same float operations in the same order. Returned parts still come from the real split(), and the original's range-dropping early return is kept for completeReduction to repair, so generated geometry is bit-identical. A new test compares the port with bezier-js exactly (Object.is) over edge cases and 200 random cubics; both digest tripwires are unchanged. Random cubics catch logic errors within the first few dozen but almost never last-ULP drift, which the edge cases catch, so a larger set would only cost CI time. At 192 pieces (16x12, Node 22): Triangles 6.1 s -> 1.75 s, Wavy 1.67 s -> 0.54 s, Classic (traced) 0.67 s -> 0.21 s. Refs #686 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
bezier-js's curveintersects/pairiteration recompute bbox() (extrema plus root finding) 2-3 times per sub-curve, and again on every call for the cached reduced parts Curve.intersect hands it. Port both: each subdivision's bbox is computed once and passed down, and the reduced parts' bboxes are cached in a WeakMap across calls. Float operations, pair order and dedup are unchanged, so results are identical; a new test compares the port with bezier-js, including on repeat calls that hit the cache, and both digest tripwires are unchanged. At 192 pieces (16x12, Node 22), on top of the reduce() port: Triangles 1.75 s -> 1.08 s, Wavy 0.54 s -> 0.41 s, Classic (traced) 0.21 s -> 0.19 s. Refs #686 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
9064e79 to
d25959c
Compare
|
Responses to the re-review: 1. Stale "2,000 cubics / 300 pairs": done. While squashing, 16520b7 was reworded to say 200 random cubics and why a larger set wouldn't add coverage. The PR description now says 200 cubics and 100 pairs. 2.
So the probe is a constructed exact tie. The digest catching the The fixups are squashed; the branch is two commits on 🤖 Generated with Claude Code |
Closes #686.
What
The traced cut styles spent most of their generation time inside bezier-js. This adds
src/puzzle/topology/bezier-js-port.ts, which ports the hot bezier-js 6.1.4 code paths without changing any results. One commit per part:reduce(): bezier-js builds several throwawayBezierobjects per 0.01 step just to asksimple()of a trial split. The port runs that check on raw control points with the same float operations in the same order. Returned parts still come from the realsplit(), and the original's range-dropping early return is kept, sincecompleteReductionrepairs it.curveintersects/pairiteration: bezier-js recomputesbbox()2–3 times per sub-curve, and again on every call for the cached reduced parts. The port computes each subdivision's bbox once and passes it down, and caches the reduced parts' bboxes in aWeakMap. Pair order and dedup are unchanged.bboxoverlapis copied rather than taken fromBezier.getUtils(), because it decides which pairspairiterationrecurses into. Taking it from the library would mix a future bezier-js's overlap test with 6.1.4's iteration.Geometry is unchanged
traced-geometry-digest,dcel-broad-phase-equivalence) pass unmodified.bezier-js-port.test.tscompares both ports with bezier-js using exactObject.isequality on every point and_t1/_t2, across edge cases and 200 random cubics. For intersection it compares 100 random curve pairs, including repeat calls that hit the cache, plus a pair whose bounding boxes touch exactly. Deliberately breaking the port (sqrt→hypot, a differentlerpformula, swapped pair order,>=→>inbboxOverlap) makes these tests fail.Timings
At 192 pieces (16×12, 1200×800), median of 3 seeds, Node 22 on an M-series Mac:
reduceportNot measured: generation through the worker in a browser, or on phones.
Test plan
npm teston Node 22 (.nvmrc): 3,646 passednpm run lint,tsc🤖 Generated with Claude Code