[Merged by Bors] - Directly copy moved Table components to the target location#5056
Closed
james7132 wants to merge 5 commits intobevyengine:mainfrom
Closed
[Merged by Bors] - Directly copy moved Table components to the target location#5056james7132 wants to merge 5 commits intobevyengine:mainfrom
james7132 wants to merge 5 commits intobevyengine:mainfrom
Conversation
james7132
added
A-ECS
Contributor
|
if #4853 gets merged, should the new functions get removed? |
Member
Author
No, these PRs are orthogonal. Though this PR lessens the impact of that PR, as |
alice-i-cecile
requested changes
Jun 21, 2022
Member
alice-i-cecile
left a comment
alice-i-cecile
left a comment
There was a problem hiding this comment.
Changes look good, and those perf results are excellent. Some suggestions on comments to help readers understand the pointer logic.
mockersf
reviewed
Jun 21, 2022
mockersf
approved these changes
Jun 21, 2022
alice-i-cecile
approved these changes
Jun 21, 2022
Member
|
bors r+ |
bors bot
pushed a commit
that referenced
this pull request
Jun 21, 2022
# Objective Speed up entity moves between tables by reducing the number of copies conducted. Currently three separate copies are conducted: `src[index] -> swap scratch`, `src[last] -> src[index]`, and `swap scratch -> dst[target]`. The first and last copies can be merged by directly using the copy `src[index] -> dst[target]`, which can save quite some time if the component(s) in question are large. ## Solution This PR does the following: - Adds `BlobVec::swap_remove_unchecked(usize, PtrMut<'_>)`, which is identical to `swap_remove_and_forget_unchecked`, but skips the `swap_scratch` and directly copies the component into the provided `PtrMut<'_>`. - Build `Column::initialize_from_unchecked(&mut Column, usize, usize)` which uses the above to directly initialize a row from another column. - Update most of the table move APIs to use `initialize_from_unchecked` instead of a combination of `swap_remove_and_forget_unchecked` and `initialize`. This is an alternative, though orthogonal, approach to achieve the same performance gains as seen in #4853. This (hopefully) shouldn't run into the same Miri limitations that said PR currently does. After this PR, `swap_remove_and_forget_unchecked` is still in use for Resources and swap_scratch likely still should be removed, so #4853 still has use, even if this PR is merged. ## Performance TODO: Microbenchmark This PR shows similar improvements to commands that add or remove table components that result in a table move. When tested on `many_cubes sphere`, some of the more command heavy systems saw notable improvements. In particular, `prepare_uniform_components<T>`, this saw a reduction in time from 1.35ms to 1.13ms (a 16.3% improvement) on my local machine, a similar if not slightly better gain than what #4853 showed [here](#4853 (comment)).  The command heavy `Extract` stage also saw a smaller overall improvement:  --- ## Changelog Added: `BlobVec::swap_remove_unchecked`. Added: `Column::initialize_from_unchecked`.
Member
|
bors r- |
Contributor
|
Canceled. |
Member
|
@BoxyUwU my impression is that this is about as simple as pointer code gets, but feel free to review this in the next few days if you want. |
alice-i-cecile
added
the
S-Ready-For-Final-Review
alice-i-cecile
approved these changes
Jun 27, 2022
Member
|
bors r+ |
bors bot
pushed a commit
that referenced
this pull request
Jun 27, 2022
# Objective Speed up entity moves between tables by reducing the number of copies conducted. Currently three separate copies are conducted: `src[index] -> swap scratch`, `src[last] -> src[index]`, and `swap scratch -> dst[target]`. The first and last copies can be merged by directly using the copy `src[index] -> dst[target]`, which can save quite some time if the component(s) in question are large. ## Solution This PR does the following: - Adds `BlobVec::swap_remove_unchecked(usize, PtrMut<'_>)`, which is identical to `swap_remove_and_forget_unchecked`, but skips the `swap_scratch` and directly copies the component into the provided `PtrMut<'_>`. - Build `Column::initialize_from_unchecked(&mut Column, usize, usize)` on top of it, which uses the above to directly initialize a row from another column. - Update most of the table move APIs to use `initialize_from_unchecked` instead of a combination of `swap_remove_and_forget_unchecked` and `initialize`. This is an alternative, though orthogonal, approach to achieve the same performance gains as seen in #4853. This (hopefully) shouldn't run into the same Miri limitations that said PR currently does. After this PR, `swap_remove_and_forget_unchecked` is still in use for Resources and swap_scratch likely still should be removed, so #4853 still has use, even if this PR is merged. ## Performance TODO: Microbenchmark This PR shows similar improvements to commands that add or remove table components that result in a table move. When tested on `many_cubes sphere`, some of the more command heavy systems saw notable improvements. In particular, `prepare_uniform_components<T>`, this saw a reduction in time from 1.35ms to 1.13ms (a 16.3% improvement) on my local machine, a similar if not slightly better gain than what #4853 showed [here](#4853 (comment)).  The command heavy `Extract` stage also saw a smaller overall improvement:  --- ## Changelog Added: `BlobVec::swap_remove_unchecked`. Added: `Column::initialize_from_unchecked`.
Contributor
bors
bot
changed the title
inodentry
pushed a commit
to IyesGames/bevy
that referenced
this pull request
Aug 8, 2022
…ne#5056) # Objective Speed up entity moves between tables by reducing the number of copies conducted. Currently three separate copies are conducted: `src[index] -> swap scratch`, `src[last] -> src[index]`, and `swap scratch -> dst[target]`. The first and last copies can be merged by directly using the copy `src[index] -> dst[target]`, which can save quite some time if the component(s) in question are large. ## Solution This PR does the following: - Adds `BlobVec::swap_remove_unchecked(usize, PtrMut<'_>)`, which is identical to `swap_remove_and_forget_unchecked`, but skips the `swap_scratch` and directly copies the component into the provided `PtrMut<'_>`. - Build `Column::initialize_from_unchecked(&mut Column, usize, usize)` on top of it, which uses the above to directly initialize a row from another column. - Update most of the table move APIs to use `initialize_from_unchecked` instead of a combination of `swap_remove_and_forget_unchecked` and `initialize`. This is an alternative, though orthogonal, approach to achieve the same performance gains as seen in bevyengine#4853. This (hopefully) shouldn't run into the same Miri limitations that said PR currently does. After this PR, `swap_remove_and_forget_unchecked` is still in use for Resources and swap_scratch likely still should be removed, so bevyengine#4853 still has use, even if this PR is merged. ## Performance TODO: Microbenchmark This PR shows similar improvements to commands that add or remove table components that result in a table move. When tested on `many_cubes sphere`, some of the more command heavy systems saw notable improvements. In particular, `prepare_uniform_components<T>`, this saw a reduction in time from 1.35ms to 1.13ms (a 16.3% improvement) on my local machine, a similar if not slightly better gain than what bevyengine#4853 showed [here](bevyengine#4853 (comment)).  The command heavy `Extract` stage also saw a smaller overall improvement:  --- ## Changelog Added: `BlobVec::swap_remove_unchecked`. Added: `Column::initialize_from_unchecked`.
james7132
added a commit
to james7132/bevy
that referenced
this pull request
Oct 28, 2022
…ne#5056) # Objective Speed up entity moves between tables by reducing the number of copies conducted. Currently three separate copies are conducted: `src[index] -> swap scratch`, `src[last] -> src[index]`, and `swap scratch -> dst[target]`. The first and last copies can be merged by directly using the copy `src[index] -> dst[target]`, which can save quite some time if the component(s) in question are large. ## Solution This PR does the following: - Adds `BlobVec::swap_remove_unchecked(usize, PtrMut<'_>)`, which is identical to `swap_remove_and_forget_unchecked`, but skips the `swap_scratch` and directly copies the component into the provided `PtrMut<'_>`. - Build `Column::initialize_from_unchecked(&mut Column, usize, usize)` on top of it, which uses the above to directly initialize a row from another column. - Update most of the table move APIs to use `initialize_from_unchecked` instead of a combination of `swap_remove_and_forget_unchecked` and `initialize`. This is an alternative, though orthogonal, approach to achieve the same performance gains as seen in bevyengine#4853. This (hopefully) shouldn't run into the same Miri limitations that said PR currently does. After this PR, `swap_remove_and_forget_unchecked` is still in use for Resources and swap_scratch likely still should be removed, so bevyengine#4853 still has use, even if this PR is merged. ## Performance TODO: Microbenchmark This PR shows similar improvements to commands that add or remove table components that result in a table move. When tested on `many_cubes sphere`, some of the more command heavy systems saw notable improvements. In particular, `prepare_uniform_components<T>`, this saw a reduction in time from 1.35ms to 1.13ms (a 16.3% improvement) on my local machine, a similar if not slightly better gain than what bevyengine#4853 showed [here](bevyengine#4853 (comment)).  The command heavy `Extract` stage also saw a smaller overall improvement:  --- ## Changelog Added: `BlobVec::swap_remove_unchecked`. Added: `Column::initialize_from_unchecked`.
ItsDoot
pushed a commit
to ItsDoot/bevy
that referenced
this pull request
Feb 1, 2023
…ne#5056) # Objective Speed up entity moves between tables by reducing the number of copies conducted. Currently three separate copies are conducted: `src[index] -> swap scratch`, `src[last] -> src[index]`, and `swap scratch -> dst[target]`. The first and last copies can be merged by directly using the copy `src[index] -> dst[target]`, which can save quite some time if the component(s) in question are large. ## Solution This PR does the following: - Adds `BlobVec::swap_remove_unchecked(usize, PtrMut<'_>)`, which is identical to `swap_remove_and_forget_unchecked`, but skips the `swap_scratch` and directly copies the component into the provided `PtrMut<'_>`. - Build `Column::initialize_from_unchecked(&mut Column, usize, usize)` on top of it, which uses the above to directly initialize a row from another column. - Update most of the table move APIs to use `initialize_from_unchecked` instead of a combination of `swap_remove_and_forget_unchecked` and `initialize`. This is an alternative, though orthogonal, approach to achieve the same performance gains as seen in bevyengine#4853. This (hopefully) shouldn't run into the same Miri limitations that said PR currently does. After this PR, `swap_remove_and_forget_unchecked` is still in use for Resources and swap_scratch likely still should be removed, so bevyengine#4853 still has use, even if this PR is merged. ## Performance TODO: Microbenchmark This PR shows similar improvements to commands that add or remove table components that result in a table move. When tested on `many_cubes sphere`, some of the more command heavy systems saw notable improvements. In particular, `prepare_uniform_components<T>`, this saw a reduction in time from 1.35ms to 1.13ms (a 16.3% improvement) on my local machine, a similar if not slightly better gain than what bevyengine#4853 showed [here](bevyengine#4853 (comment)).  The command heavy `Extract` stage also saw a smaller overall improvement:  --- ## Changelog Added: `BlobVec::swap_remove_unchecked`. Added: `Column::initialize_from_unchecked`.
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
4 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
Speed up entity moves between tables by reducing the number of copies conducted. Currently three separate copies are conducted:
src[index] -> swap scratch,src[last] -> src[index], andswap scratch -> dst[target]. The first and last copies can be merged by directly using the copysrc[index] -> dst[target], which can save quite some time if the component(s) in question are large.Solution
This PR does the following:
BlobVec::swap_remove_unchecked(usize, PtrMut<'_>), which is identical toswap_remove_and_forget_unchecked, but skips theswap_scratchand directly copies the component into the providedPtrMut<'_>.Column::initialize_from_unchecked(&mut Column, usize, usize)on top of it, which uses the above to directly initialize a row from another column.initialize_from_uncheckedinstead of a combination ofswap_remove_and_forget_uncheckedandinitialize.This is an alternative, though orthogonal, approach to achieve the same performance gains as seen in #4853. This (hopefully) shouldn't run into the same Miri limitations that said PR currently does. After this PR,
swap_remove_and_forget_uncheckedis still in use for Resources and swap_scratch likely still should be removed, so #4853 still has use, even if this PR is merged.Performance
TODO: Microbenchmark
This PR shows similar improvements to commands that add or remove table components that result in a table move. When tested on
many_cubes sphere, some of the more command heavy systems saw notable improvements. In particular,prepare_uniform_components<T>, this saw a reduction in time from 1.35ms to 1.13ms (a 16.3% improvement) on my local machine, a similar if not slightly better gain than what #4853 showed here.The command heavy
Extractstage also saw a smaller overall improvement:Changelog
Added:
BlobVec::swap_remove_unchecked.Added:
Column::initialize_from_unchecked.