Skip to content

Opportunistically use dense iter for archetypal iteration in Par_iter #14673

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Sep 3, 2024
Merged

Opportunistically use dense iter for archetypal iteration in Par_iter #14673

merged 7 commits into from
Sep 3, 2024

Conversation

re0312
Copy link
Contributor

@re0312 re0312 commented Aug 9, 2024

Objective

Performance

image

no performance regression for regular itertaion

3.5X faster in hybrid parallel iteraion,this number is far greater than the benefits obtained in regular iteration(~1.81) because mutable iterations on continuous memory can effectively reduce the cost of mataining core cache coherence

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Performance A change motivated by improving speed, memory usage or compile times S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Aug 9, 2024
@alice-i-cecile alice-i-cecile added this to the 0.15 milestone Aug 9, 2024
@alice-i-cecile alice-i-cecile added the M-Needs-Release-Note Work that should be called out in the blog due to impact label Aug 9, 2024
@alice-i-cecile
Copy link
Member

Dang, those are some impressive numbers.

chescock
chescock approved these changes Aug 9, 2024
View reviewed changes
NthTensor
NthTensor approved these changes Sep 3, 2024
View reviewed changes
Copy link
Contributor

@NthTensor NthTensor left a comment

Choose a reason for hiding this comment

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

I don't feel qualified to approve the code but I did replicate the results.

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 3, 2024
13ros27
13ros27 approved these changes Sep 3, 2024
View reviewed changes
Copy link
Contributor

@13ros27 13ros27 left a comment

Choose a reason for hiding this comment

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

This looks good to me, I didn't run the benchmarks or scrutinise them very much but I trust your and Nths numbers. The comments are very clear and easy to follow so thank you for those, nice bit of deduplication too.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Sep 3, 2024
Merged via the queue into bevyengine:main with commit 739007f Sep 3, 2024
26 checks passed
@alice-i-cecile alice-i-cecile mentioned this pull request Oct 20, 2024
Closed
@alice-i-cecile
Copy link
Member

Thank you to everyone involved with the authoring or reviewing of this PR! This work is relatively important and needs release notes! Head over to bevyengine/bevy-website#1682 if you'd like to help out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@chescock chescock chescock approved these changes

@NthTensor NthTensor NthTensor approved these changes

@13ros27 13ros27 13ros27 approved these changes

Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events 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-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Milestone
0.15
Development

Successfully merging this pull request may close these issues.
5 participants
@re0312 @alice-i-cecile @chescock @NthTensor @13ros27