-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
implement FromEntitySetIterator #17513
implement FromEntitySetIterator #17513
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm… this is slower for me:
let mut world = World::new();
for _ in 0..1_000_000 {
world.spawn_empty();
}
let mut query = world.query::<Entity>();
for _ in 0..10 {
query.iter(&world).collect::<EntityHashSet>();
}
gives me
Time (mean ± σ): 81.1 ms ± 0.9 ms [User: 56.1 ms, System: 24.5 ms]
Range (min … max): 80.0 ms … 83.7 ms 36 runs
while doing the same with collect_set
gives
Time (mean ± σ): 221.8 ms ± 4.3 ms [User: 108.1 ms, System: 112.2 ms]
Range (min … max): 217.2 ms … 231.5 ms 13 runs
This seems to be because it doesn't use size_hint
. With that fixed it improves to
Time (mean ± σ): 79.1 ms ± 2.3 ms [User: 54.8 ms, System: 23.9 ms]
Range (min … max): 77.7 ms … 92.1 ms 37 runs
or slightly faster than the standard collect
(the spawning part of this benchmark takes about 26ms, with this PR improving the iter/collect part from ~49ms to ~47ms). Not a huge difference :/
Use size hint in the HashSet FromEntitySetIterator impls Co-authored-by: SpecificProtagonist <[email protected]>
good catch! |
What system/compilation settings did you use to bench this? |
I've also changed the for loop into a fold, since internal iteration is known to generate bit better code. |
# Objective In #16547, we added `EntitySet`s/`EntitySetIterator`s. We can know whenever an iterator only contains unique entities, however we do not yet have the ability to collect and reuse these without either the unsafe `UniqueEntityIter::from_iterator_unchecked`, or the expensive `HashSet::from_iter`. An important piece for being able to do this is a `Vec` that maintains the uniqueness property, can be collected into, and is itself `EntitySet`. A lot of entity collections are already intended to be "unique", but have no way of expressing that when stored, other than using an aforementioned `HashSet`. Such a type helps by limiting or even removing the need for unsafe on the user side when not using a validated `Set` type, and makes it easier to interface with other infrastructure like f.e. `RelationshipSourceCollection`s. ## Solution We implement `UniqueEntityVec`. This is a wrapper around `Vec`, that only ever contains unique elements. It mirrors the API of `Vec`, however restricts any mutation as to not violate the uniqueness guarantee. Meaning: - Any inherent method which can introduce new elements or mutate existing ones is now unsafe, f.e.: `insert`, `retain_mut` - Methods that are impossible to use safely are omitted, f.e.: `fill`, `extend_from_within` A handful of the unsafe methods can do element-wise mutation (`retain_mut`, `dedup_by`), which can be an unwind safety hazard were the element-wise operation to panic. For those methods, we require that each individual execution of the operation upholds uniqueness, not just the entire method as a whole. To be safe for mutable usage, slicing and the associated slice methods require a matching `UniqueEntitySlice` type , which we leave for a follow-up PR. Because this type will deref into the `UniqueEntitySlice` type, we also offer the immutable `Vec` methods on this type (which only amount to a handful). "as inner" functionality is covered by additional `as_vec`/`as_mut_vec` methods + `AsRef`/`Borrow` trait impls. Like `UniqueEntityIter::from_iterator_unchecked`, this type has a `from_vec_unchecked` method as well. The canonical way to safely obtain this type however is via `EntitySetIterator::collect_set` or `UniqueEntityVec::from_entity_set_iter`. Like mentioned in #17513, these are named suboptimally until supertrait item shadowing arrives, since a normal `collect` will still run equality checks.
# Objective Some collections are more efficient to construct when we know that every element is unique in advance. We have `EntitySetIterator`s from bevyengine#16547, but currently no API to safely make use of them this way. ## Solution Add `FromEntitySetIterator` as a subtrait to `FromIterator`, and implement it for the `EntityHashSet`/`hashbrown::HashSet` types. To match the normal `FromIterator`, we also add a `EntitySetIterator::collect_set` method. It'd be better if these methods could shadow `from_iter` and `collect` completely, but rust-lang/rust#89151 is needed for that. While currently only `HashSet`s implement this trait, future `UniqueEntityVec`/`UniqueEntitySlice` functionality comes with more implementors. Because `HashMap`s are collected from tuples instead of singular types, implementing this same optimization for them is more complex, and has to be done separately. ## Showcase This is basically a free speedup for collecting `EntityHashSet`s! ```rust pub fn collect_milk_dippers(dippers: Query<Entity, (With<Milk>, With<Cookies>)>) { dippers.iter().collect_set::<EntityHashSet>(); // or EntityHashSet::from_entity_set_iter(dippers); } --------- Co-authored-by: SpecificProtagonist <[email protected]>
# Objective In bevyengine#16547, we added `EntitySet`s/`EntitySetIterator`s. We can know whenever an iterator only contains unique entities, however we do not yet have the ability to collect and reuse these without either the unsafe `UniqueEntityIter::from_iterator_unchecked`, or the expensive `HashSet::from_iter`. An important piece for being able to do this is a `Vec` that maintains the uniqueness property, can be collected into, and is itself `EntitySet`. A lot of entity collections are already intended to be "unique", but have no way of expressing that when stored, other than using an aforementioned `HashSet`. Such a type helps by limiting or even removing the need for unsafe on the user side when not using a validated `Set` type, and makes it easier to interface with other infrastructure like f.e. `RelationshipSourceCollection`s. ## Solution We implement `UniqueEntityVec`. This is a wrapper around `Vec`, that only ever contains unique elements. It mirrors the API of `Vec`, however restricts any mutation as to not violate the uniqueness guarantee. Meaning: - Any inherent method which can introduce new elements or mutate existing ones is now unsafe, f.e.: `insert`, `retain_mut` - Methods that are impossible to use safely are omitted, f.e.: `fill`, `extend_from_within` A handful of the unsafe methods can do element-wise mutation (`retain_mut`, `dedup_by`), which can be an unwind safety hazard were the element-wise operation to panic. For those methods, we require that each individual execution of the operation upholds uniqueness, not just the entire method as a whole. To be safe for mutable usage, slicing and the associated slice methods require a matching `UniqueEntitySlice` type , which we leave for a follow-up PR. Because this type will deref into the `UniqueEntitySlice` type, we also offer the immutable `Vec` methods on this type (which only amount to a handful). "as inner" functionality is covered by additional `as_vec`/`as_mut_vec` methods + `AsRef`/`Borrow` trait impls. Like `UniqueEntityIter::from_iterator_unchecked`, this type has a `from_vec_unchecked` method as well. The canonical way to safely obtain this type however is via `EntitySetIterator::collect_set` or `UniqueEntityVec::from_entity_set_iter`. Like mentioned in bevyengine#17513, these are named suboptimally until supertrait item shadowing arrives, since a normal `collect` will still run equality checks.
Objective
Some collections are more efficient to construct when we know that every element is unique in advance.
We have
EntitySetIterator
s from #16547, but currently no API to safely make use of them this way.Solution
Add
FromEntitySetIterator
as a subtrait toFromIterator
, and implement it for theEntityHashSet
/hashbrown::HashSet
types.To match the normal
FromIterator
, we also add aEntitySetIterator::collect_set
method.It'd be better if these methods could shadow
from_iter
andcollect
completely, but rust-lang/rust#89151 is needed for that.While currently only
HashSet
s implement this trait, futureUniqueEntityVec
/UniqueEntitySlice
functionality comes with more implementors.Because
HashMap
s are collected from tuples instead of singular types, implementing this same optimization for them is more complex, and has to be done separately.Showcase
This is basically a free speedup for collecting
EntityHashSet
s!