Override QueryIter::fold to port Query::for_each perf gains to select Iterator combinators#6773
Merged
james7132 merged 30 commits intobevyengine:mainfrom Dec 1, 2023
Merged
Conversation
james7132
added
A-ECS
Contributor
|
Oh, man, this hadn't happened yet? It came up on Discord back in Feb 2021; I guess I should have opened an issue. As an FYI, there's a chance that implementing |
Member
Author
|
Finally was able to address the performance issues, so I'm taking this out of draft. Benchmark results: Details |
james7132
added
the
C-Code-Quality
alice-i-cecile
approved these changes
Feb 18, 2023
Member
alice-i-cecile
left a comment
alice-i-cecile
left a comment
There was a problem hiding this comment.
Great safety comments. I think the unsafe is absolutely worth it to close this gap. Agreed on the deprecation.
Member
|
@InBetweenNames, I'd appreciate your review on this :) |
joseph-gio
approved these changes
Feb 18, 2023
Member
joseph-gio
left a comment
joseph-gio
left a comment
There was a problem hiding this comment.
Very nice work, it's good to see this get improved upon. I especially like how the folding behavior was split into two separate functions for fold_table and fold_archetype -- it feels much cleaner this way.
Co-authored-by: JoJoJet <[email protected]>
cart
approved these changes
Feb 19, 2023
Member
cart
left a comment
cart
left a comment
There was a problem hiding this comment.
I'm on board for this change and the code looks good to me. Finally deprecating for_each!
|
Yeah this does make sense to me. The #6161 PR can be adapted to use the new style. |
joseph-gio
reviewed
Nov 26, 2023
Co-authored-by: Joseph <[email protected]>
alice-i-cecile
added
the
S-Ready-For-Final-Review
Member
|
@james7132, unless I hear otherwise, I'm going to merge this tomorrow :) |
james7132
added a commit
to james7132/bevy_asm_tests
that referenced
this pull request
Nov 26, 2023
Member
Author
|
As a final sanity check, the codegen of this PR seems to show no tangible difference from what is in |
joseph-gio
reviewed
Nov 27, 2023
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Contributor
|
Changelog needs updating btw. Also, this is the first I'm hearing of it, but using for_each() instead for |
Member
|
@james7132 sorry, you lost the coin flip on the merge conflicts :( Merge it yourself when you're done <3 |
Contributor
|
@JMS55 Roughly, But it's easy to come up with examples where the Query iterator is non-trivial and the work to be done is simple, and thus using |
james7132
force-pushed
the
query-iter-fold
branch
from
3db7137 to
541f4c5
Compare
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
10 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Objective
After #6547,
Query::for_eachhas been capable of automatic vectorization on certain queries, which is seeing a notable (>50% CPU time improvements) for iteration. However,Query::for_eachisn't idiomatic Rust, and lacks the flexibility of iterator combinators.Ideally,
Query::iterand friends should be able to achieve the same results. However, this does seem to blocked upstream (rust-lang/rust#104914) by Rust's loop optimizations.Solution
This is an intermediate solution and refactor. This moves the
Query::for_eachimplementation onto theIterator::foldimplementation forQueryIterinstead. This should result in the same automatic vectorization optimization on allIteratorfunctions that internally use fold, includingIterator::for_each,Iterator::count, etc.With this, it should close the gap between the two completely. Internally, this PR changes
Query::for_eachto usequery.iter().for_each(..)instead of the duplicated implementation.Separately, the duplicate implementations of internal iteration (i.e.
Query::par_for_each) now use portions of the currentQuery::for_eachimplementation factored out into their own functions.This also massively cleans up our internal fragmentation of internal iteration options, deduplicating the iteration code used in
for_eachandpar_iter().for_each().Changelog
Changed:
Query::for_each,Query::for_each_mut,Query::for_each, andQuery::for_each_muthave been moved toQueryIter'sIterator::for_eachimplementation, and still retains their performance improvements over normal iteration. These APIs are deprecated in 0.13 and will be removed in 0.14.