Override QueryIter::fold to port Query::for_each perf gains to select Iterator combinators #6773
Conversation
This file contains 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
james7132
added
A-ECS
|
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 |
|
Finally was able to address the performance issues, so I'm taking this out of draft. Benchmark results: |
james7132
added
the
C-Code-Quality
alice-i-cecile
approved these changes
Feb 18, 2023
|
@InBetweenNames, I'd appreciate your review on this :) |
JoJoJet
approved these changes
Feb 18, 2023
Co-authored-by: JoJoJet <21144246+JoJoJet@users.noreply.github.com>
cart
approved these changes
Feb 19, 2023
|
Yeah this does make sense to me. The #6161 PR can be adapted to use the new style. |
JoJoJet
reviewed
Nov 26, 2023
Co-authored-by: Joseph <21144246+JoJoJet@users.noreply.github.com>
alice-i-cecile
added
the
S-Ready-For-Final-Review
|
@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
|
As a final sanity check, the codegen of this PR seems to show no tangible difference from what is in |
JoJoJet
reviewed
Nov 27, 2023
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
Changelog needs updating btw. Also, this is the first I'm hearing of it, but using for_each() instead for |
|
@james7132 sorry, you lost the coin flip on the merge conflicts :( Merge it yourself when you're done <3 |
|
@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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.