Optimize the 2D shape hot path - #9025
Conversation
Per-vertex costs in the 2.x shape system dominate 2D drawing of dense paths (3-4x slower than 1.x on fill-heavy scenes). Keep the shape architecture intact but remove per-vertex overhead: - Shape.vertex(): fast path that appends consecutive line vertices into a single multi-vertex LineSegment on PATH contours, skipping the creator-map lookup and generic capacity/merge logic. Closing vertices still get their own segment so isClosing semantics are unchanged. Batching also amortizes the draw-side per-segment visitor dispatch, which measurement shows is the dominant cost on dense paths: with every other optimization applied but batching disabled, fill-heavy scenes still run 47-71% slower than with it. - PrimitiveToPath2DConverter/PrimitiveToVerticesConverter iterate a segment's vertices, so batched segments render identically. - Vertex constructor: Object.assign() instead of Object.entries(), avoiding a per-vertex entries array allocation. - Converters take hasFill/hasStroke so invisible paths are skipped. Adds unit tests covering the batched fast path: accumulation into one segment, isClosing isolation, and non-PATH contours taking the general path.
The 2D primitive methods (rect, ellipse, arc, line, ...) built a fresh p5.Shape per call. Reuse a per-renderer scratch Shape reset between calls instead, and pass hasFill/hasStroke through to the converter so arc() does not build Path2D geometry that will never be painted (a stroke-only arc previously constructed and discarded its fill path, and vice versa).
Per review on processing#8973: creator construction and lookup should stay fast since reusable p5.Shape objects are planned. Store creators in a module-level nested plain object keyed by vertex kind then shape kind, so a lookup is two property accesses with no key string to build (previously `vertex-${kind}` spliced per lookup into a Map key), and nothing is constructed per Shape (previously a Map plus a dozen closures per instance). Shape kinds are primitive constants, which work as computed keys; Symbols would too, if constants become Symbols later. PrimitiveShapeCreators was not exported and only Shape's default path constructed it, so the class is removed outright. Benched neutral on 2D scenes (the vertex() fast path already skips lookups on the hot path); the win is construction cost and simplicity.
|
@VANSH3104, tagging you as a co-reviewer since this builds on the shape-pipeline work. |
davepagurek
left a comment
davepagurek
left a comment
There was a problem hiding this comment.
Thanks for working on this! The changes generally look good. The main question I had is whether we could avoid special casing vertex calls and still get the batching benefits.
| class LineSegment extends Segment { | ||
| #vertexCapacity = 1; | ||
|
|
||
| // Consecutive line vertices on a path batch into a single LineSegment |
There was a problem hiding this comment.
Was the capacity of 1 preventing it from creating a polyline before? addToShape already has code for merging a new shape with a previous one, could we have been relying on that rather than special-casing calls to vertex?
it stays at 1 so the generic path never merges into a LineSegment
What's an example of some user code inside beginShape/endShape that would trigger that if we made this Infinity?
There was a problem hiding this comment.
Any consecutive vertex() calls would trigger it, for example:
beginShape();
vertex(0, 0);
vertex(20, 0);
vertex(20, 20);
endShape(CLOSE);With Infinity, the third vertex merges into the existing LineSegment; the closing segment still has to remain separate. I tried that generic path, but it made the issue snippet about 51% slower, so direct batching worth keeping.
There was a problem hiding this comment.
Does that imply that there was a large overhead just in making a new LineSegment before then merging it?
There was a problem hiding this comment.
(If so, doesn't need to block this PR, but it's maybe worth making another issue for that to clean up the special case introduced here and make the default case faster)
| const visitor = new PrimitiveToPath2DConverter({ | ||
| strokeWeight: this.states.strokeWeight | ||
| strokeWeight: this.states.strokeWeight, | ||
| hasFill: !!this.states.fillColor || this._clipping, |
There was a problem hiding this comment.
Remind me how clipping works here? is there a scenario where you can clip to just the fill or just to the stroke that we need to handle?
There was a problem hiding this comment.
Clipping only uses the base path, so there isn’t a fill-only or stroke-only case here. I’ve updated this to skip building those paths while clipping.
| // line, ...). Each call fully consumes the shape via drawShape() before | ||
| // returning, so a single reusable instance per renderer avoids | ||
| // rebuilding a Shape (and its vertex-property closures) per call. | ||
| _primitiveShape() { |
There was a problem hiding this comment.
This optimization changes the lifetime semantics of the Shape passed to drawShape(). Previously each primitive call created a fresh Shape, whereas now the same scratch Shape is reset and reused. This breaks consumers that retain the Shape beyond the duration of drawShape(). Would it be worth keeping the existing lifecycle and instead seeing what state can be cached statically rather than on the instance, so we preserve the previous semantics while still getting most of the performance benefits?
There was a problem hiding this comment.
actually it looks like the new caching is on Renderer, so the main cost of doing a new shape is this loop:
for (const key in this.#vertexProperties) {
if (key !== 'position' && key !== 'textureCoordinates') {
this[key] = function(value) {
this.#vertexProperties[key] = value;
};
}
}Is that a bottleneck? if not then we could be good already
There was a problem hiding this comment.
Yes. I rechecked, and fresh Shapes aren’t a bottleneck here. I’ve restored the original lifetime and removed the scratch reuse.
Renderer hooks may retain the Shape passed to drawShape(), so preserve the existing per-call lifetime. Clipping uses only the converter base path, so skip building specialized fill and stroke paths there.
davepagurek
left a comment
davepagurek
left a comment
There was a problem hiding this comment.
Thanks for your work on this! Let me know what you think about the possibility of cleaning up the special-case added here, we can file another issue/PR for it if that seems possible.
Optimize the 2D shape hot path
|
@all-contributors please add @okra-sf for code |
|
I've put up a pull request to add @okra-sf! 🎉 |
4 participants
Resolves #8973
Changes:
Dense 2D paths currently create and visit one
LineSegmentper vertex. This PR keeps the existing shape architecture, but:LineSegment; closing vertices remain separate, preservingCLOSEbehavior;Shapeand a static creator table for 2D primitives instead of rebuilding that setup on every call; andPath2Dconstruction for arcs when that path will not be drawn.Performance against current main at
5491e3c(median ms/frame, Chromium; lower is better):The benchmark report includes the runner, raw repeat runs, and the ablations discussed in #8973. One tradeoff from the ablation: batching by itself costs about 12% on the stroke-only scene. The other changes more than offset it, so the complete branch is 1.4× faster there.
There are no public API changes. New unit tests cover batching boundaries, including
CLOSEand non-PATHmodes. Locally:PR Checklist
npm run lintpassesAI disclosure: I used Claude and Codex for analysis, testing, and review. I reviewed and take responsibility for all changes.