perf(modeling): fix O(n²) performance issues in flatten, splitPolygonByPlane, and extrudeFromSlices#1429
Conversation
|
I am extra cautious regarding AI assisted code. I do use it with my ow preferences what I allow it to do. This PR and issues look reasonable, and well guided by @jbroll . I was always bad at doing reviews, so this as well as many other PR's will have to be approved by @z3dev. I am liking what I am seeing here regardless of my guarded/cautious stance on AI coding agents. |
|
I actually didn't mean for this PR to become public quite yet, the agent made the pr against upstream instead of my local repo. if you like I'm happy to break this up into individual PRs for each optimization |
I personally would not require you to do that, so please keep it as is, until @z3dev finds time to review it :) |
jbroll
force-pushed
the
perf/modeling-optimizations
branch
from
558ddb4 to
14f6cfe
Compare
|
@jbroll Nice! Performance improvements are always welcome. Thanks! Please drop the small performance tests as there aren’t any performance suites for the CI. There’s a test suite under JSCAD Community. You can try this with these changes to get an understanding of the performance impact. https://github.com/jscad-community/jscad-performance Also, V3 has performance changes. So, these changes should be integrated in the future. I typically work on V3, and integrate changes into V2 if important. You’re welcome to work on either version currently but there will be a day that V3 becomes master. |
|
@jbroll Fun stuff! I reviewed most of the changes which look good. There are a few changes that I need to understand better. I’ll complete a full review this weekend. Please keep the PR scope small as that makes the reviews easy and quick. |
Replace recursive concat-based flatten with iterative stack-based implementation. This eliminates the O(n²) behavior caused by creating a new array on every concat call. Benchmark results: - flatten-flat-100: 38µs → 8.6µs (4.4x faster) - flatten-flat-1000: 2130µs → 80µs (26x faster) - flatten-mixed-20x20: 94µs → 14µs (6.6x faster) Scaling is now linear (O(n)) instead of quadratic. The iterative approach also avoids stack overflow on deeply nested arrays and doesn't hit spread operator limits. Fixes jscad#1422 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Adds comprehensive test coverage for the flatten utility function: - Empty and flat arrays - Single and deep nesting - Mixed nesting depths - Empty nested arrays - Order preservation - Object reference preservation - Various types (primitives, objects, functions) - Large arrays (1000+ items) - Input immutability Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…oval Replace splice-in-loop pattern with a helper function that builds a filtered array in O(n) time. Before: splice() is O(n) called in a loop = O(n²) After: Single pass building new array = O(n) Benchmark improvement: - subtract-sphere-16: 4.27ms → 1.69ms (2.5x faster) - subtract-sphere-32: 10.41ms → 8.50ms (1.2x faster) This is in the hot path for all CSG boolean operations. Fixes jscad#1421 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Adds direct test coverage for the polygon splitting function: - Coplanar-front detection (type 0) - Polygon entirely in front (type 2) - Polygon entirely in back (type 3) - Spanning polygon split (type 4) - Duplicate vertex removal verification - Complex polygon splitting This provides regression protection for the performance fix in jscad#1421. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Replace O(n²) array concatenation pattern with O(n) push operations. Each concat() creates a new array and copies all elements. In a loop with N iterations, this results in O(n²) total copying. Using push() with a for-loop avoids this overhead. Uses for-loop instead of spread operator to avoid stack limits with large polygon arrays (as noted by @hrgdavor in jscad#1420). Fixes jscad#1419 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Fix O(n²) splice in reTesselateCoplanarPolygons with mark-and-filter - Reuse mat4 for constant X rotation in extrudeRotate - Reuse vec3 increment in extrudeWalls repartitionEdges - Replace spread with loop in retessellate to prevent stack overflow - Add unit tests for all optimizations - Add benchmark suite for primitives and boolean operations Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Replace immediate splice with lazy cleanup to eliminate O(n²) cost when many nodes are removed during boolean operations. - Remove indexOf + splice from remove() - just mark node as removed - Add lazy compaction in getPolygons() to clean up dead nodes - All iteration paths already skip removed nodes via isRemoved() or polygon !== null checks Performance improvement on union operations: - union-sphere-32: 26.9ms -> 10.5ms (61% faster) - union-sphere-64: 238ms -> 89ms (63% faster) No memory impact - dead nodes cleaned up when getPolygons() called. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add comment explaining that getPolygons() is only called on root node via Tree.allPolygons(), addressing code review feedback about children array mutation safety. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
jbroll
force-pushed
the
perf/modeling-optimizations
branch
from
14f6cfe to
bf6fbf5
Compare
- splitPolygonByPlane: move vertices.length < 3 check to call sites to avoid function invocation overhead per z3dev's feedback - PolygonTreeNode: replace .filter() with for loop for faster compaction on large polygon sets per z3dev's feedback Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Per z3dev's suggestion, replace the stack-based implementation with the native Array.flat(Infinity) method. This is simpler and equally performant for JSCAD's use cases (normalizing variadic arguments). Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
4 participants
Summary
This PR addresses several O(n²) performance issues identified through code review and benchmarking:
concatwith iterative stack-based approach → 26x faster on large arrayssplicein loop with filtered array construction → 2.5x faster on subtract operationsconcatin loop withpush→ O(n²) → O(n)Also includes:
npm run bench) for performance regression testingflatten(13 tests) andsplitPolygonByPlane(6 tests)Benchmark Results
flatten (used by 48 files across codebase)
Boolean operations (splitPolygonByPlane fix)
Test plan
Related Issues
Fixes #1419, #1421, #1422
Note: #1427 was closed as invalid (poly3.toPoints already returns direct reference)
🤖 Generated with Claude Code