[Merged by Bors] - [ecs] Improve Commands performance#2332
Closed
NathanSWard wants to merge 25 commits intobevyengine:mainfrom
Closed
[Merged by Bors] - [ecs] Improve Commands performance#2332NathanSWard wants to merge 25 commits intobevyengine:mainfrom
Commands performance#2332NathanSWard wants to merge 25 commits intobevyengine:mainfrom
Conversation
NathanSWard
added
A-ECS
Contributor
Author
|
Here is a list of the minimal performance tests I created. |
Contributor
Author
|
I have a feeling that since this is a pretty specialized structure I should probably just move it into |
NathanSWard
commented
Jun 11, 2021
Member
|
Hmm, should we maybe pull the commands benchmark into a separate PR and merge it earlier? It's far less controversial than the unsafe anystack impl, and reducing the scope of PR s with unsafe is usually a good thing |
DJMcNab
approved these changes
Jun 12, 2021
Member
DJMcNab
left a comment
DJMcNab
left a comment
There was a problem hiding this comment.
I really like the idea of using an fn here. Apart from the alignment issue you have pointed out, this looks good.
NathanSWard
force-pushed
the
nward/commands-no-alloc
branch
from
6a179d1 to
5d6db0f
Compare
DJMcNab
approved these changes
Jun 12, 2021
Member
DJMcNab
left a comment
DJMcNab
left a comment
There was a problem hiding this comment.
I haven't checked over the tests in detail, but if this passes miri, I can believe it's good to go.
Contributor
Author
I'll go ahead and run miri with this sometime today/tomorrow and report back the results. |
DJMcNab
approved these changes
Jun 13, 2021
Member
DJMcNab
left a comment
DJMcNab
left a comment
There was a problem hiding this comment.
Looks good, a few docs thoughts
NathanSWard
force-pushed
the
nward/commands-no-alloc
branch
from
51b2fe0 to
0d75c6b
Compare
Contributor
Author
|
Here are the most recent benchmarks (taken from #2334). |
NathanSWard
force-pushed
the
nward/commands-no-alloc
branch
from
543522c to
2fb8a5a
Compare
Contributor
Author
|
As @mockersf suggested I created a benchmark that utilized |
NathanSWard
force-pushed
the
nward/commands-no-alloc
branch
3 times, most recently
from
ae297f1 to
2a614b8
Compare
Contributor
Author
|
benchmarks for the most recent version. (with special casing for 0 sized commands and ensuring the vector isn't null). |
bjorn3
reviewed
Jun 15, 2021
bjorn3
approved these changes
Jun 15, 2021
NathanSWard
force-pushed
the
nward/commands-no-alloc
branch
from
9dbe32f to
a9be103
Compare
Contributor
Author
|
@cart this should be good to go now :) |
Member
|
bors r+ |
bors bot
pushed a commit
that referenced
this pull request
Jul 16, 2021
# Objective
- Currently `Commands` are quite slow due to the need to allocate for each command and wrap it in a `Box<dyn Command>`.
- For example:
```rust
fn my_system(mut cmds: Commands) {
cmds.spawn().insert(42).insert(3.14);
}
```
will have 3 separate `Box<dyn Command>` that need to be allocated and ran.
## Solution
- Utilize a specialized data structure keyed `CommandQueueInner`.
- The purpose of `CommandQueueInner` is to hold a collection of commands in contiguous memory.
- This allows us to store each `Command` type contiguously in memory and quickly iterate through them and apply the `Command::write` trait function to each element.
Contributor
bors
bot
changed the title
Commands performanceCommands performance
ostwilkens
pushed a commit
to ostwilkens/bevy
that referenced
this pull request
Jul 27, 2021
# Objective
- Currently `Commands` are quite slow due to the need to allocate for each command and wrap it in a `Box<dyn Command>`.
- For example:
```rust
fn my_system(mut cmds: Commands) {
cmds.spawn().insert(42).insert(3.14);
}
```
will have 3 separate `Box<dyn Command>` that need to be allocated and ran.
## Solution
- Utilize a specialized data structure keyed `CommandQueueInner`.
- The purpose of `CommandQueueInner` is to hold a collection of commands in contiguous memory.
- This allows us to store each `Command` type contiguously in memory and quickly iterate through them and apply the `Command::write` trait function to each element.
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
9 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
Commandsare quite slow due to the need to allocate for each command and wrap it in aBox<dyn Command>.will have 3 separate
Box<dyn Command>that need to be allocated and ran.Solution
CommandQueueInner.CommandQueueInneris to hold a collection of commands in contiguous memory.Commandtype contiguously in memory and quickly iterate through them and apply theCommand::writetrait function to each element.