Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parallel Transform Propagation #17840

Open
wants to merge 37 commits into
base: main
Choose a base branch
from

Conversation

aevyrie
Copy link
Member

@aevyrie aevyrie commented Feb 13, 2025

Objective

  • Make transform propagation faster.

Solution

  • Work sharing worker threads
  • Parallel tree traversal excluding leaves
  • Second cache friendly wide pass over all leaves
  • 3-10x faster than main

Testing

  • Tracy
  • Caldera hotel is showing 7x faster on my M4 Max - big caveat - these numbers seem to shift wildly run to run, so I don't know that I would advertise a particular number. But it Is faster in a... statistically significant way.
    image

@aevyrie aevyrie requested a review from Victoronz February 13, 2025 06:36
@alice-i-cecile alice-i-cecile added this to the 0.16 milestone Feb 13, 2025
@alice-i-cecile alice-i-cecile added C-Performance A change motivated by improving speed, memory usage or compile times A-Transform Translations, rotations and scales M-Needs-Release-Note Work that should be called out in the blog due to impact S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Feb 13, 2025
@aevyrie aevyrie marked this pull request as draft February 13, 2025 06:49
@aevyrie
Copy link
Member Author

aevyrie commented Feb 13, 2025

Getting this up now that it seems to function, marking as draft because it still needs cleanup.

@alice-i-cecile
Copy link
Member

FYI, I'm merging #17815. The methods there are probably worth benching here.

@alice-i-cecile
Copy link
Member

I'd love a before / after tracy histogram BTW. I've seen enough tests on Discord to be convinced that this is markedly (3x or so) faster, but it would be lovely to show in the release notes.

@aevyrie aevyrie marked this pull request as ready for review February 17, 2025 20:13
Copy link
Contributor

@superdump superdump left a comment

Choose a reason for hiding this comment

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

It looks complicated at first glance, but it's actually not so bad, especially because of the comments on all the non-obvious stuff. Awesome improvements! Thanks for your efforts. And I'm glad we have unit tests for this. :D

@superdump superdump requested a review from james7132 February 17, 2025 21:22
Copy link
Contributor

@hymm hymm left a comment

Choose a reason for hiding this comment

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

Nice work. Overall makes sense to me, but I'm going to give this another pass tomorrow and try and understand the logic a little better.

Copy link
Contributor

@pcwalton pcwalton left a comment

Choose a reason for hiding this comment

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

Looks good, just had a few questions. Overall thanks a ton for doing this work, it really helps!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Transform Translations, rotations and scales C-Performance A change motivated by improving speed, memory usage or compile times M-Needs-Release-Note Work that should be called out in the blog due to impact S-Needs-Review Needs reviewer attention (from anyone!) to move forward
Projects
None yet
Development

Successfully merging this pull request may close these issues.