Scratch param

[loreball]

Objective

Introduce a system param for collections holding scratch data.
Closes #23775

Solution

Implement two new items to bevy_ecs: Scratch and ClearableCollection. Scratch is the new system param for hodling scratch data. It holds a ClearableCollection which it clears on every system run.

I went with a trait based solution instead of ScratchVec<T> to prevent duplication of the capacity management code. That said I tried to not over-complicate the capacity management code itself. It currently just runs a 10 000 tick timer that resets every time the allocation capacity grows. If it runs down we try to halve the allocation size.

Testing

The PR comes with 2 new tests, but I still want to benchmark how using this affects render asset extraction (which is why this pr is a draft for now).

---

Showcase

TODO

[loreball]

There was a suggestion on the original issue to make Scratch<T> always Send since it's always empty when running a system. We can't do that. We can't run custom SystemParam code after a system runs so we do the clearing when creating the param for a system run. If the system fills the collection on thread A and then runs on thread B, we'll end up dropping the data on thread B. Dropping !Send data on a different thread could lead to UB.

[beicause]

We can't run custom SystemParam code after a system runs so we do the clearing when creating the param for a system run.

Is it possible to overcome this?

Also Scratch<T> should not require Sync like Local<T> does.

[loreball]

Doing a quick benchmark it seems pretty neutral for asset extraction.

(Ignore the +2% my system is pretty noisy I've seen it jump +- 4%)

[loreball]

We can't run custom SystemParam code after a system runs so we do the clearing when creating the param for a system run.

Is it possible to overcome this?

I don't think so. Not at least while maintaining panic safety.

The way it would work is we would add a new method to SystemParam and friends. Let's call it post_system_run. Then we'd add a new safety requirement to get_param. post_system_run must always be run between calls to get_param. So let's take look at where we run a system

// ...
let params = unsafe {
    F::Param::get_param(&mut state.param, &self.system_meta, world, change_tick)
}?;

/// <ignore hotpatching impl>
let out = self.func.run(input, params);

/// We would add a `post_system_run` call here

self.system_meta.last_run = change_tick;

If self.func.run panics we'd never execute post_system_run. Catching the panic would then allow you to trigger the whole thing twice. The standard solution is to define a new struct that maintains safety invariants on drop, since panics run drop code. This is where our own permissiveness bites us in the ass. The SystemParam::Item is allowed to borrow the SystemParam::State meaning our drop handler which also needs mutable access to the state cannot be alive before the system runs.

[loreball]

I'm pulling this out of draft since this version is mostly done. I'll hold off on writing the showcase until I get at least one review. Hopefully, the safety issue can be hashed out and we get to push the best versions of this across the finish line.

[beicause]

Local does not require Sync, why Scratch require it?

[Bluefinger]

I wonder if we can improve this for the first system run case, because that seems like a bit of a sore point. I don't think we have to worry about the overflow case, but the first run feels like something we do want to worry about.

[loreball]

The code fires before the system gets to execute for the first time. Most collections will not have allocated at this point. The shrinking will be a no-op.

We could switch to counting system runs. This would fix the unnecessary shrinks, but it would mean wasting more memory on systems that use run conditions.

[Bluefinger]

Then it is probably not a case to worry about, if it just becomes a no-op. I was just wondering if there was a path for better, but if the trade-offs aren't worth, then let's just keep it as is (as it is documented anyway).