General entity set cleanup #21498
General entity set cleanup #21498
Conversation
Victoronz
added
C-Docs
Victoronz
force-pushed
the
general-entity-set-cleanup
branch
from
380de8d to
35b16db
Compare
|
removed use of nightly const |
Victoronz
force-pushed
the
general-entity-set-cleanup
branch
from
b06f603 to
a1530b0
Compare
| impl<I: Iterator<Item: EntityEquivalent>> Iterator for UniqueEntityIter<I> { | ||
| type Item = I::Item; | ||
|
|
||
| #[inline] |
There was a problem hiding this comment.
It doesn't make sense to inline non-public functions or functions with generics. I'd add inline only to things that are expected to be used often, not to every small function.
Please, see the standard library developers guide and this article.
There was a problem hiding this comment.
Hmm, seems like std strongly diverges from this advice. Given that the vast majority of entity set code simply delegates somewhere else, I thought taking over those inlining decisions + being more liberal with them because these functions are small would be feasible, but that doesn't seem to be the case.
I now also realize what the inline-more annotations in hashbrown mean.
Judging by the discussions around inline, I wouldn't say that it doesn't make sense to inline non-public functions or functions with generics, but that it makes sense less often.
Given this, I think I'll cull most #[inline]s, aside from hotter functions, and those that I know can miss optimization opportunities, mostly collects and extends.
Do you think that, ignoring the standard library, I should retain the inlining decisions of the most popular crates that either define/wrap data structures like this code area does?
Even with this new knowledge, some inlining in those crates feel somewhat arbitrary or like plain oversights.
There was a problem hiding this comment.
The linked article explains it quite nicely. Trivial private functions can already be inlined because the compiler sees their bodies during crate compilation. We even got a conservative cross-crate inlining recently. Generics are monomorphized during compilation, so their bodies are also visible during crate compilation, which means the compiler can inline them as well. inline just hints to the compiler that this function might be used often and suggests inlining even without LTO (though the compiler can still ignore this).
It would be annoying to manually add inline to every small function. So unless profiled or used really often, I'd avoid adding inline at all.
It's like unrolling loops - we don't do this anymore, unless profiling shows that it's necessary for humans to step in.
| /// `iter` must only yield unique elements. | ||
| /// As in, the resulting iterator must adhere to the safety contract of [`EntitySetIterator`]. | ||
| #[inline] | ||
| pub const unsafe fn from_iter_ref_unchecked(iter: &I) -> &Self { |
There was a problem hiding this comment.
I guess this makes sense for completeness, but there's not much you can do with an &impl Iterator.
Same thing for UniqueEntityEquivalentVec::from_vec_ref_unchecked. Are there ever any cases where you need that instead of UniqueEntityEquivalentSlice::from_slice_unchecked? I guess you can call capacity()?
There was a problem hiding this comment.
For iterators, remember that is a UniqueEntityIter construction method, its main purpose is to be able to mark any iterator as an EntitySetIterator. This is not just for iteration itself! Sometimes, there are &SomeIterator can return underlying views into a collection, like f.e. as_slice()/AsRef<[T]>.
For Vecs, it has to do with safety around the uniqueness invariant:
If you know that you have borrowed the full collection, you have stronger guarantees about its subsections.
F.e. a slice can always have adjacent elements you have no awareness of/access to. If you have a Vec, this is never the case.
Right now, mutable UniqueEntitySlice logic is not yet implemented, so we do not yet have safety comments that talk about this subtlety.
Interestingly enough, the need to reference collections while maintaining a "no superslice" guarantee is not one I've really heard of before, which seems to be corroborated by some triggered lints surrounding &Box<[T]> and the like.
| #[inline] | ||
| pub const unsafe fn from_keys_unchecked<S>( | ||
| keys: hash_map::Keys<'a, Entity, V>, | ||
| ) -> Keys<'a, V, S> { |
There was a problem hiding this comment.
It might make sense to put this in a separate impl block so that it can use Self. (And similarly for the other from_foo_unchecked methods.)
Oh, or is the point to be able to call it as Keys::from_keys_unchecked<SomeType> instead of Keys::<_, SomeType>::from_keys_unchecked?
| pub const unsafe fn from_keys_unchecked<S>( | ||
| keys: hash_map::Keys<'a, Entity, V>, | ||
| ) -> Keys<'a, V, S> { | ||
| Keys::<'_, _, S>(keys, PhantomData) |
There was a problem hiding this comment.
The S can be inferred from the return type, right?
| Keys::<'_, _, S>(keys, PhantomData) | |
| Keys(keys, PhantomData) |
(And similarly for the other from_foo_unchecked methods.)
| } | ||
|
|
||
| /// Returns the number of elements in the set. | ||
| pub fn len(&self) -> usize { |
There was a problem hiding this comment.
This got removed because it's available through Deref, right? Why not also remove is_empty()?
What about UniqueEntityEquivalentVec::len()? ... Huh, Vec defines that separately from the deref to slice. I wonder why.
There was a problem hiding this comment.
is_empty wasn't removed simply because I missed it!
The Vec::len story is an interesting tidbit:
With the majority of newtypes, you implement a deref into the inner type, sparing you the need to implement the immutable methods on your wrapper.
But we didn't just wrap Vec, we wrapped its deref &[T] as well!
Hence, we can't use the usual delegation pattern to &Vec.
As for why Vec::len is separate from [T]::len, good question, I don't know! Maybe historical?
Even if we could delegate it, I think the inconsistency would lead to more confusion than removing one function is worth.
3 participants
Objective
There are various straightforward cleanup/code quality changes that can be done in the entity set logic.
Solution
These changes tend to be far-reaching and bitrotty, so this PR stacks several of them. It should be reviewed commit after commit!
Commit 1: Privatizing entity set type fields
- Sometimes, other places in the engine would make use of the crate-public nature of some of these types to circumvent
encapsulation, which risks circumventing safety requirements as well.
Commit 2: entity set constructor consistency
- There were some missing constructors that serve as more direct and consistent code paths into entity set logic.
Commit 3: inline entity set logic
- When authoring the entirety of
EntitySet, I just... completely forgot about any inlining! They had no semantic impact, so Ididn't think of them XD.
- The inlines here are best-effort, and try to roughly match the inlining decisions
stdmakes. Sometimes, those decisions aremysteries, or might be downright suboptimal, but that is a lot of effort to check.
(Looking at you,
binary_search/binary_search_by...)- This should result in a small general performance gain for code based on this logic.
LIkely noticeable with
EntityHashMap/Set``propagate_transforms.Commit 4: stop internally skipping entity set type construction safety
- This mostly serves as documentation for further contributors/users, clarifying why certain impls exist, and why the API
should not be circumvented.
Commit 5: Turn various entity set functions const
Commit 6: miscellaneous simplifications
Commit 7: doc cleanup
indexmapdoc links are broken, given that they work when I generate the docs locally. Soleft them as is for now
Commit 8: update bevy ecs msrv to 1.89, and the associated clippy fixes.
There is more to do, like add methods that have since been added to the various types, and cleanup/inlining surrounding query iteration methods directly, but I wanted to restrict myself to the
entitymodule for now. This PR frees me up to do other entity set changes with less worry about cleanup conflicts.Testing
These code quality/consistency changes should not change semantics, outside of adding a few missing constructors.