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).

[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?

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.

Is it feasible to implement Drop for Scratch and call clear on drop, instead of on get_param? Since Scratch should be dropped even if the system panics after running.

[loreball]

Is it feasible to implement Drop for Scratch and call clear on drop, instead of on get_param? Since Scratch should be dropped even if the system panics after running.

mem::forget is safe. Safety can't depend on destructors being ran.

[coolcatcoder]

The word Attempts is doing some heavy lifting here.
It would be nice to add an extra line reassuring users that not shrinking the capacity is allowed.

[coolcatcoder]

Consider adding default bodies for capacity and shrink_to, to support the common case where the implementation is clearing a non-collection.
Alternatively you could add a non-collection example to the documentation.

[coolcatcoder]

Perhaps 10 frames should be the default of an associated const in the trait ClearableCollection.

[urben1680]

While being possible overridable on the param:

Scratch<T, const DEFAULT_SHRINK_DELAY: usize = <T as ClearableCollection>::DEFAULT_SHRINK_DELAY>

(if this compiles)

[coolcatcoder]

Unfortunately that would require the unstable feature min_generic_const_args.