[Merged by Bors] - reduce tricky unsafety and simplify table structure #2221
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
|
For completeness, here are my benchmark results. Just note that my machine is fairly noisy. |
NathanSWard
added
A-ECS
bjorn3
approved these changes
May 20, 2021
NathanSWard
suggested changes
May 20, 2021
NathanSWard
approved these changes
May 20, 2021
NathanSWard
added
the
S-Ready-For-Final-Review
cart
reviewed
May 24, 2021
crates/bevy_ecs/src/storage/table.rs
Outdated
| #[inline] | ||
| fn ticks_mut(&mut self) -> &mut Vec<ComponentTicks> { | ||
| // SAFE: unique access | ||
| unsafe { &mut *self.ticks.get() } |
There was a problem hiding this comment.
We can make this safe using self.ticks.get_mut()
There was a problem hiding this comment.
Oh cool! I for some reason was unable to find this method. I see it's stabilised since 1.50.
Note that this UnsafeCell goes away in #2227 completely.
|
I am getting a reproducible |
|
bors r+ |
bors bot
pushed a commit
that referenced
this pull request
May 24, 2021
I've noticed that we are overusing interior mutability of the Table data, where in many cases we already own a unique reference to it. That prompted a slight refactor aiming to reduce number of safety constraints that must be manually upheld. Now the majority of those are just about avoiding bound checking, which is relatively easy to prove right. Another aspect is reducing the complexity of Table struct. Notably, we don't ever use archetypes stored there, so this whole thing goes away. Capacity and grow amount were mostly superficial, as we are already using Vecs inside anyway, so I've got rid of those too. Now the overall table capacity is being driven by the internal entity Vec capacity. This has a side effect of automatically implementing exponential growth pattern for BitVecs reallocations inside Table, which to my measurements slightly improves performance in tests that are heavy on inserts. YMMV, but I hope that those tests were at least remotely correct.
|
Pull request successfully merged into main. Build succeeded: |
bors
bot
changed the title
bors bot
pushed a commit
that referenced
this pull request
May 30, 2021
Continuing the work on reducing the safety footguns in the code, I've removed one extra `UnsafeCell` in favour of safe `Cell` usage inisde `ComponentTicks`. That change led to discovery of misbehaving component insert logic, where data wasn't properly dropped when overwritten. Apart from that being fixed, some method names were changed to better convey the "initialize new allocation" and "replace existing allocation" semantic. Depends on #2221, I will rebase this PR after the dependency is merged. For now, review just the last commit. Co-authored-by: Carter Anderson <mcanders1@gmail.com>
ostwilkens
pushed a commit
to ostwilkens/bevy
that referenced
this pull request
Jul 27, 2021
I've noticed that we are overusing interior mutability of the Table data, where in many cases we already own a unique reference to it. That prompted a slight refactor aiming to reduce number of safety constraints that must be manually upheld. Now the majority of those are just about avoiding bound checking, which is relatively easy to prove right. Another aspect is reducing the complexity of Table struct. Notably, we don't ever use archetypes stored there, so this whole thing goes away. Capacity and grow amount were mostly superficial, as we are already using Vecs inside anyway, so I've got rid of those too. Now the overall table capacity is being driven by the internal entity Vec capacity. This has a side effect of automatically implementing exponential growth pattern for BitVecs reallocations inside Table, which to my measurements slightly improves performance in tests that are heavy on inserts. YMMV, but I hope that those tests were at least remotely correct.
ostwilkens
pushed a commit
to ostwilkens/bevy
that referenced
this pull request
Jul 27, 2021
Continuing the work on reducing the safety footguns in the code, I've removed one extra `UnsafeCell` in favour of safe `Cell` usage inisde `ComponentTicks`. That change led to discovery of misbehaving component insert logic, where data wasn't properly dropped when overwritten. Apart from that being fixed, some method names were changed to better convey the "initialize new allocation" and "replace existing allocation" semantic. Depends on bevyengine#2221, I will rebase this PR after the dependency is merged. For now, review just the last commit. Co-authored-by: Carter Anderson <mcanders1@gmail.com>
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.
I've noticed that we are overusing interior mutability of the Table data, where in many cases we already own a unique reference to it. That prompted a slight refactor aiming to reduce number of safety constraints that must be manually upheld. Now the majority of those are just about avoiding bound checking, which is relatively easy to prove right.
Another aspect is reducing the complexity of Table struct. Notably, we don't ever use archetypes stored there, so this whole thing goes away. Capacity and grow amount were mostly superficial, as we are already using Vecs inside anyway, so I've got rid of those too. Now the overall table capacity is being driven by the internal entity Vec capacity. This has a side effect of automatically implementing exponential growth pattern for BitVecs reallocations inside Table, which to my measurements slightly improves performance in tests that are heavy on inserts. YMMV, but I hope that those tests were at least remotely correct.