Garbage collect event memory

[james7132]

Objective

As mentioned briefly in discussions in bevyengine/rfcs#53, the memory used for events is never garbage collected despite the data being stored transiently. Bevy should release that memory when possible to avoid said memory from being held for too long.

Solution

Use Vec::shrink_to to halve the capacity of the the backing Vec for events after clearing during updates.

[DJMcNab]

The problem with anything like this is that certain workloads will have a ping-pong effect of allocating, deallocating and reallocating. Have we checked what the total amount of capacity being taken up by this is in a reasonable workflow?

For example, moving the mouse will stop for at least a few frames, then as soon as the mouse starts moving again, we'd have to allocate again (potentially reallocating too).

[james7132]

The primary motivator here is RFC linked above, which suggests we move hierarchy alteration detection to an event instead of incurring heavier archetype moves via PreviousParent instead. This will lead to large bursts of events being introduced as scenes are loaded, scaling linearly with the number of entities loaded, which would otherwise never be reclaimed without this.

The actual shrink should be pretty cheap given the Vec is emptied before the reallocation.

Given that events tend to be either streamed in at a consistent pace (roughly the same number of events per frame) or sent in bursts, potentially many frames apart, I think this strategy accounts for both fairly well. In the consistent case, capacity is retained to roughly the current usage, otherwise if it isn't being used it exponentially decays every two frames. In the aforementioned mouse example, the movements per frame will trigger a consistent flow of events frame to frame, requiring only reallocations on the ramp up. As it falls off, the capacity decays, but provides a window for it to more easily ramp up if need be.

If we don't do this automatically, we should at least provide the tools/APIs for allowing users to manually do it themselves. IMO it's irresponsible of us to permanently allocate memory on behalf of the user, and provide them no ways to rein it in.

[cart]

Yeah I think this is reasonable. I think something like this is good as a starting point to ensure memory is reclaimed (which does feel like table stakes).

That being said, providing a user-configurable "off button" for garbage collection would make me more comfortable merging this. I'm relatively certain that there will be cases that would benefit from that, and it seems pretty easy to add in.

Something like:

enum EventGarbageCollection {
  ExponentialFallOff {
    /// Each internal vec never goes below this value.
    /// There are two internal vecs, so the total minimum capacity is 2 * min_capacity.
    min_capacity: usize,
  },
  None,
}

Note that I added min_capacity because it seems like it would be an easy add and would give users (and us) an additional tool to avoid reallocations.

We can provide additional configuration and tweaks to optimize as needed.

[james7132]

Should we have this on every Events<T> resource or as a global resource? and how would we configure this at initialization?

[cart]

Should we have this on every Events resource or as a global resource? and how would we configure this at initialization?

I think it should be for each Events<T> resource, as each event type will have different allocation patterns.

I'm thinking there are two paths we could take:

1. Store the config on each Events<T> resource. add_event::<T> just sets it to some default value. Configuring it would involve grabbing the resource and modifying the config on it.
2. Add an EventSettings<T> resource (like we do for plugins). Pass that in to Events::update inside Events::update_system.

As a riff on (2), we could consider replacing add_event::<T>() with a more standard add_plugin(EventPlugin::<T>::default()) (or keep the extension method, but register a plugin inside add_event::<T>().

I think (2) is more "idiomatic bevy", so that has my preference. But I'm open to suggestions.

[alice-i-cecile]

This 100% needs to be controlled separately for each event-type due to differing usage patterns. User-controllable defaults might be good though.

Approach 3: use an Event trait, and configure it in there. We had previously discussed this for e.g. smallvec storage, and it helps improve type-safety and automatic documentation of code.

The downside to that style of design of course is that it's fixed at the type level and can't be configured by users or dynamically modified. I think those are serious limitations, and will interfere with the ability to automatically explore micro-optimizations down the line in user code.

Of all these options, I think I prefer approach 2, so we're not fetching the config data every single time. And moving this to a more standard plugin model is good, but we should keep the convenience method (or do autoregistration).

[alice-i-cecile]

I really like this flavor of design; there are a number of other settings that I'd like to be able to configure for events (e.g. smallvec usage and cleanup strategy).

As a result, I think we should refactor add_event to use a generic plugin under the hood.

[alice-i-cecile]

Suggested change

	/// Exponentially decays the amount of allocated memory for the backing event buffer
	/// Halves the amount of allocated memory for the backing event buffer each tick

[alice-i-cecile]

This is a nice use of an optional resource; I like the way it avoids polluting the resource list with default values.

[djeedai]

my 2c: This strategy although interesting for some scenarios is probably too aggressive to be the default one, and we should consider a smoother version kicking in only after a few frames. And as always, if the caller knows for a fact there's an atypical pattern on one frame, we should offer an escape hatch to manually trigger a GC, as the caller always has more context to decide what's best.

One thing also: here the proposed scheme will fall flat if some recurring bursts occur alternatively on frame A and frame B (e.g. every 3 frames), as we only look at the size of the current buffer to decide if it needs to shrink. We should probably have at least a slightly more complex heuristic that looks at both buffers to determine an expected capacity for next frame.

Also, I'm not sure what order of magnitude of memory allocations we're talking about here, but if this is a strong concern, we should maybe consider using a single circular buffer instead of 2 swapping vecs. The advantage is that there's a single allocation covering 2 frames together, so it will more easily absorb bursts and other ping-pong effects, and it naturally solves the issue of the 2-buffer heuristic mentioned above. The ideal data structure is probably similar to a deque, with blocks of N items (to reduce the number of allocations), and the possibility to add more blocks to handle bursts (here, only at one end, unlike deque).

[djeedai]

Does default() works here (and below)? Or we need the extra Default:: qualification?

[djeedai]

I don't know how Vec::shrink_to() is implemented, but if the length of the vec decreases slowly frame-to-frame, this will cause a (non no-op) call to Vec::shrink_to() every frame, which potentially will lead to a reallocation every frame depending on the implementation of Vec::shrink_to() (which I imagine has some kind of threshold). This should probably be at least tested / validated with various sizes to understand the behavior, because if the implementation is a bit naive and it ends up doing one allocation per frame then that defeats the purpose of having reusable buffers in the first place.

More generally the exponential decay here is very aggressive. Given the cost of allocations compared the relative benefit of recovering memory, it's probably worth having a smoother heuristic which observes the trend over several frames before kicking in some GC operation. Or at least make that an option, and make that option the default one, and leave that aggressive instant-trigger decay variant for those few Events<T> which we know for a fact are prone to large bursts for which we do not want to pay the cost of a large unused allocation, and instead prefer paying the cost of a reallocation. But I don't think reallocating as soon as possible should be the default.

[bas-ie]

Closing due to inactivity as part of backlog cleanup, and treating the linked RFC bevyengine/rfcs#53 as the tracking issue. cc: @alice-i-cecile in case I got this one wrong, bit above my pay grade 🙂