Remove the need for collect_inner() on QuerySortedManyIter

[chescock]

Objective

When calling any of the sort methods on a QueryManyIter with mutable data, collect_inner() must be called before fetching items. Remove the need for that call, improving both ergonomics and performance.

Solution

The reason for collect_inner() was to avoid aliasing issues between the L::Items used during sorting and the D::Items returned from fetching. Instead, we wrap the L::Item in a MaybeUninit to avoid aliasing issues.

L::Item and D::Item may have conflicting access, so we must ensure that we never have both alive for the same entity. The same entity may appear multiple times in self.entity_iter, so we must ensure that no L::Item is alive when we create D::Items in fetch_next. The entity_list we return is a Map<vec::IntoIter> that owns the contents of keyed_query, so it would keep the L::Items alive if we store them in keyed_query directly.

The safest solution is to collect() the Map into a fresh Vec<Entity>, but we want to avoid the overhead of doing a new allocation and copying the data. We could re-use a single allocation by storing Option<L::Item> and setting them to None, but that would still have overhead of writing the None values.

Instead, we store MaybeUninit<L::Item>, which acts similar to Option. The compiler can never assume the item is initialized, so we don't need to do anything to uninitialize it.

(If this works, it can also help unblock #15396. One of the issues there is that the sort() methods hold a L::Item that would borrow from the query state. If we can make those be uninitialized MaybeUninit<L::Item>s, then we could safely transmute away the lifetimes.)

Testing

Added a test that creates a QuerySortedManyIter over mutable data with duplicate Entitys. Without the MaybeUninit wrapper, running it through miri fails with an aliasing violation. With the MaybeUninit wrapper, it passes.

[Victoronz]

I don't think this works?
To call MaybeUninit::new(T) you must have already created that T, and when it is a reference, it's borrow is not undone by being placed in MaybeUninit after construction. Neither does MaybeUninit::assume_init_drop do what we want, since a Copy value can still be used after calling ptr::drop_in_place, which is what assume_init_drop does.

[chescock]

I don't think this works?
To call MaybeUninit::new(T) you must have already created that T, and when it is a reference, it's borrow is not undone by being placed in MaybeUninit after construction

It does work! That's what the miri test checks. I'm happy to add more tests if you can think of a case I missed.

Lifetimes are erased in the abstract machine, so a borrow only needs to be valid when it's part of a live value. If the original lifetime mattered then the existing code would already be unsound, since it creates L::Item<'w> values that alias the D::Item<'w> values.

Once you place values in a MaybeUninit, they no longer have to be valid, because MaybeUninit is always valid. So the borrow essentially is undone. It's only if you later call an assume_init method that it will need to have been valid.

neither does MaybeUninit::assume_init_drop do anything

It's true that assume_init_drop doesn't affect the safety here! That's just to prevent leaks for query items that do have drop glue.

Would you think it would work if we used Option and explicitly set them to None? What if we used MaybeUninit but had an explicit *item = MaybeUninit::uninit(); assignment?

[chescock]

For what it's worth, MaybeDangling from RFC 3336 would better match the semantics we need here and be a little more clear. The text of that RFC discusses some similar situations and might help clarify what can cause UB here.

[Victoronz]

It does work! That's what the miri test checks. I'm happy to add more tests if you can think of a case I missed.

The miri test should spawn more than 1 item, otherwise the sort doesn't have to do any comparisons/worry about aliasing.

While it does sound more convincing, I want read up more on the safety of this first.

It's true that assume_init_drop doesn't affect the safety here! That's just to prevent leaks for query items that do have drop glue.

True!

Setting aside safety, it is true that we want to avoid the allocation from a collect, but the sorts themselves are still the most expensive part in all of this, and degrading their performance would make the gain moot.
It might make more sense to just eat the collect cost.
It would be a lot easier to judge if we had some benches for these sorts, but creating useful benches for sorts is usually difficult.

[chescock]

The miri test should spawn more than 1 item, otherwise the sort doesn't have to do any comparisons/worry about aliasing.

I do pass that one entity twice in each sort (.iter_many_mut(&mut world, [id, id])). So it has to compare the first copy to the second one, and it has to be sure to drop the lens for the second copy before fetching the data for the first one.

What would you like the test would do with the other entity? Sort [id1, id1, id2, id2]?

Setting aside safety, it is true that we want to avoid the allocation from a collect, but the sorts themselves are still the most expensive part in all of this, and degrading their performance would make the gain moot.
It might make more sense to just eat the collect cost.
It would be a lot easier to judge if we had some benches for these sorts, but creating useful benches for sorts is usually difficult.

I wouldn't expect any of these changes to impact performance, since the new code is entirely no-ops at runtime. But it's hard to tell without testing!

Using collect() is certainly easier to prove safe than what I'm doing here! If the performance cost is small then it might be worth doing that instead. I will say that one of my goals here is to eventually address #15396 (comment), so whatever we do for QueryManyIter::sort() I'd like to also do to QueryIter::sort().

[Victoronz]

Using collect() is certainly easier to prove safe than what I'm doing here! If the performance cost is small then it might be worth doing that instead.

Let's do it the simpler way and just do the collect! I think I'm more comfortable with that after dwelling on it for a bit.

I will say that one of my goals here is to eventually address #15396 (comment), so whatever we do for QueryManyIter::sort() I'd like to also do to QueryIter::sort().

The collect_inner issue in particular is only present for Many style iteration, the normal QueryIters have a uniqueness guarantee for the entities they iterate over.

[chescock]

Let's do it the simpler way and just do the collect! I think I'm more comfortable with that after dwelling on it for a bit.

Alright, I'll put together a PR for that!

The collect_inner issue in particular is only present for Many style iteration, the normal QueryIters have a uniqueness guarantee for the entities they iterate over.

Yup! But in #15396 I want to let query items borrow from the state. The lens state is a local variable, so that means ending the lens borrows before returning. So whatever we do to end the lens borrows in QueryManyIter, I'll later want to do the same thing for a different reason to end the lens borrows in QueryIter.

[Victoronz]

Yup! But in #15396 I want to let query items borrow from the state. The lens state is a local variable, so that means ending the lens borrows before returning. So whatever we do to end the lens borrows in QueryManyIter, I'll later want to do the same thing for a different reason to end the lens borrows in QueryIter.

Yeah, for the QueryManyIter sorts it makes sense to do it on its own because of collect_inner, but for doing it on QueryIter I'd prefer it in the same PR as the query state borrowing itself, so that we only do that if/once that PR goes through!