Skip to content
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.

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

Reduce branching when inserting components #8053

Merged
merged 10 commits into from
Mar 21, 2023

Conversation

james7132
Copy link
Member

@james7132 james7132 commented Mar 12, 2023

Objective

We're currently using an unconditional unwrap in multiple locations when inserting bundles into an entity when we know it will never fail. This adds a large amount of extra branching that could be avoided on in release builds.

Solution

Use DebugCheckedUnwrap in bundle insertion code where relevant. Add and update the safety comments to match.

This should remove the panicking branches from release builds, which has a significant impact on the generated code: https://github.com/james7132/bevy_asm_tests/compare/less-panicking-bundles#diff-e55a27cfb1615846ed3b6472f15a1aed66ed394d3d0739b3117f95cf90f46951R2086 shows about a 10% reduction in the number of generated instructions for EntityMut::insert, EntityMut::remove, EntityMut::take, and related functions.

@james7132 james7132 added A-ECS Entities, components, systems, and events C-Performance A change motivated by improving speed, memory usage or compile times labels Mar 12, 2023
@james7132 james7132 added this to the 0.11 milestone Mar 12, 2023
@james7132 james7132 requested a review from JoJoJet March 12, 2023 14:08
@alice-i-cecile alice-i-cecile added the S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help label Mar 12, 2023
@james7132
Copy link
Member Author

Definitely seems like there's a significant gain here. Microbenchmark results below:

group                                           main                                    reduced-branching-bundles
-----                                           ----                                    -------------------------
add_remove/sparse_set                           1.01   751.5±37.73µs        ? ?/sec     1.00   746.1±41.38µs        ? ?/sec
add_remove/table                                1.00  1122.5±18.47µs        ? ?/sec     1.00  1127.5±32.19µs        ? ?/sec
add_remove_big/sparse_set                       1.04  863.7±260.23µs        ? ?/sec     1.00  830.1±167.62µs        ? ?/sec
add_remove_big/table                            1.05      2.6±0.13ms        ? ?/sec     1.00      2.5±0.12ms        ? ?/sec
get_or_spawn/batched                            1.23   367.7±10.81µs        ? ?/sec     1.00   298.6±11.58µs        ? ?/sec
get_or_spawn/individual                         1.12   538.0±41.37µs        ? ?/sec     1.00   478.7±44.37µs        ? ?/sec
insert_commands/insert                          1.23   447.2±38.92µs        ? ?/sec     1.00   362.9±26.15µs        ? ?/sec
insert_commands/insert_batch                    1.33   385.9±16.20µs        ? ?/sec     1.00   290.3±17.74µs        ? ?/sec
insert_simple/base                              1.14    428.0±3.58µs        ? ?/sec     1.00    375.4±2.38µs        ? ?/sec
insert_simple/unbatched                         1.06   796.8±31.34µs        ? ?/sec     1.00   751.6±38.02µs        ? ?/sec
spawn_commands/2000_entities                    1.00    183.6±8.21µs        ? ?/sec     1.00    182.9±7.45µs        ? ?/sec
spawn_commands/4000_entities                    1.02   365.3±11.42µs        ? ?/sec     1.00    357.8±9.71µs        ? ?/sec
spawn_commands/6000_entities                    1.04   563.4±34.24µs        ? ?/sec     1.00   542.3±20.69µs        ? ?/sec
spawn_commands/8000_entities                    1.02   733.0±29.96µs        ? ?/sec     1.00   716.7±20.49µs        ? ?/sec
spawn_world/10000_entities                      1.00  841.9±117.49µs        ? ?/sec     1.10  929.8±168.96µs        ? ?/sec
spawn_world/1000_entities                       1.00    84.2±10.69µs        ? ?/sec     1.09    92.0±13.85µs        ? ?/sec
spawn_world/100_entities                        1.00      8.4±1.18µs        ? ?/sec     1.13      9.5±1.23µs        ? ?/sec
spawn_world/10_entities                         1.00  850.8±115.52ns        ? ?/sec     1.11  948.4±149.29ns        ? ?/sec
spawn_world/1_entities                          1.00    91.6±11.54ns        ? ?/sec     1.03    94.7±10.75ns        ? ?/sec

Separately noticed that there's still a few potential out-of-bounds panics when creating a BundleInserter or BundleSpawner that could be removed as well, but we can wait to do that in a separate PR.

@james7132 james7132 removed the S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help label Mar 13, 2023
@jdm
Copy link
Contributor

jdm commented Mar 13, 2023

It's interesting that all of the benchmarks are a bit faster, except for the spawn_world ones which are all consistently a bit slower.

@james7132
Copy link
Member Author

It's interesting that all of the benchmarks are a bit faster, except for the spawn_world ones which are all consistently a bit slower.

I'm not sure where this is coming from. The only change that might affect it in this PR is the changes to write_components.

@james7132 james7132 requested a review from JoJoJet March 13, 2023 02:58
Copy link
Member

@JoJoJet JoJoJet left a comment

Choose a reason for hiding this comment

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

Thank you for fixing the invariant. I think this PR is in a good state now, got a couple more suggestions though.

crates/bevy_ecs/src/bundle.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/bundle.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/bundle.rs Show resolved Hide resolved
james7132 and others added 2 commits March 13, 2023 19:53
Co-authored-by: JoJoJet <21144246+JoJoJet@users.noreply.github.com>
@JoJoJet JoJoJet requested review from hymm and DJMcNab March 17, 2023 22:09
@cart cart enabled auto-merge March 21, 2023 20:26
@cart cart added this pull request to the merge queue Mar 21, 2023
@cart cart merged commit 6dda873 into bevyengine:main Mar 21, 2023
@james7132 james7132 deleted the less-panicking-bundles branch March 21, 2023 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Performance A change motivated by improving speed, memory usage or compile times
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants