Skip to content

perf(Renderer): accumulate actors and volumes without concat - #3638

Merged
finetjul merged 2 commits into
Kitware:masterfrom
PaulHax:upstream-renderer-actors
Sep 10, 2026
Merged

finetjul merged 2 commits into
Kitware:masterfrom
PaulHax:upstream-renderer-actors

Conversation

@PaulHax

@PaulHax PaulHax commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

Context

getActors() and getVolumes() collect each prop's objects using repeated
concat calls. Each concat call copies the growing result array.
This quadratic work repeats on every getActors call, including getActors()
during picking without an explicit pick list.

Batching geometry into fewer actors is the vtk.js way, but separate
actors are useful when datasets load and unload independently. This change came
out of loading tiled point clouds, where chunks of points are swapped in and out as the view changes.

Results

Recorded headless Chromium medians per collection call, spanning actor and
volume results:

Count Before After
400 0.05–0.06 ms 0.005–0.010 ms
2,000 1.08–1.21 ms 0.035–0.055 ms

At 2,000 objects, this saves roughly 1 ms per call.

Changes

  • Use a shared push helper for linear collection, preserving array and
    non-array prop answers.
  • Preserve order and fresh snapshots; by-reference getters return the latest
    snapshot.

PR Checklist

  • GitHub Actions CI passed: semantic-release commit messages, lint, and tests
  • Test coverage added
  • Documentation and TypeScript definitions are updated to match these changes

@sankhesh sankhesh left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

// Append array elements individually; append other values as one element.
function collectFromProps(props, getFromProp) {
const collected = [];
props.forEach((prop) => {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sometimes .forEach is slower than a regular for loop. It might not be true each time though (Chrome, Firefox)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, adopted. Its significantly faster in FireFox.

Comment thread Sources/Rendering/Core/Renderer/index.js

@finetjul finetjul left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@finetjul
finetjul added this pull request to the merge queue Sep 10, 2026
Merged via the queue into Kitware:master with commit 8ecf4d1 Sep 10, 2026
3 checks passed
@github-actions

Copy link
Copy Markdown

🎉 This PR is included in version 36.14.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions github-actions Bot added the released Automated label label Sep 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

released Automated label

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants