Skip to content
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

Garbage collect event memory #4832

Closed
wants to merge 4 commits into from
Closed
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 63 additions & 13 deletions crates/bevy_ecs/src/event.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,41 @@ struct EventInstance<E: Event> {
pub event: E,
}

/// Settings for controlling the general behavior of [`Events<T>`].
pub struct EventSettings<T> {
pub garbage_collection: EventGarbageCollection,
marker_: PhantomData<fn() -> T>,
}

impl<T> Default for EventSettings<T> {
fn default() -> Self {
Self {
garbage_collection: Default::default(),
marker_: PhantomData,
}
}
}

/// Settings for controlling the garbage collection behavior of [`Events<T>`].
pub enum EventGarbageCollection {
Copy link
Member

@alice-i-cecile alice-i-cecile Jun 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

/// Exponentially decays the amount of allocated memory for the backing event buffer
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/// with a configured minimum. This is the default.
ExponentialFalloff {
/// Each internal vec never goes below this value.
/// There are two internal vecs, so the total minimum capacity is 2 * min_capacity.
/// This defaults to 0.
min_capacity: usize,
},
/// Disables garbage collection entirely.
None,
}

impl Default for EventGarbageCollection {
fn default() -> Self {
Self::ExponentialFalloff { min_capacity: 0 }
}
}

/// An event collection that represents the events that occurred within the last two
/// [`Events::update`] calls.
/// Events can be written to using an [`EventWriter`]
Expand All @@ -82,7 +117,7 @@ struct EventInstance<E: Event> {
///
/// # Example
/// ```
/// use bevy_ecs::event::Events;
/// use bevy_ecs::event::*;
///
/// struct MyEvent {
/// value: usize
Expand All @@ -91,9 +126,10 @@ struct EventInstance<E: Event> {
/// // setup
/// let mut events = Events::<MyEvent>::default();
/// let mut reader = events.get_reader();
/// let settings = EventSettings::<MyEvent>::default();
///
/// // run this once per update/frame
/// events.update();
/// events.update(&settings);
///
/// // somewhere else: send an event
/// events.send(MyEvent { value: 1 });
Expand Down Expand Up @@ -437,9 +473,19 @@ impl<E: Event> Events<E> {

/// Swaps the event buffers and clears the oldest event buffer. In general, this should be
/// called once per frame/update.
pub fn update(&mut self) {
pub fn update(&mut self, settings: &EventSettings<E>) {
std::mem::swap(&mut self.events_a, &mut self.events_b);
// Garbage collect unused event space. Shrink after clear to avoid a copy.
let new_capacity = match settings.garbage_collection {
EventGarbageCollection::ExponentialFalloff { min_capacity } => self
.events_b
.len()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

.max(self.events_b.capacity() / 2)
.max(min_capacity),
EventGarbageCollection::None => self.events_b.capacity(),
};
self.events_b.clear();
self.events_b.shrink_to(new_capacity);
self.events_b.start_event_count = self.event_count;
debug_assert_eq!(
self.events_a.start_event_count + self.events_a.len(),
Expand All @@ -448,8 +494,12 @@ impl<E: Event> Events<E> {
}

/// A system that calls [`Events::update`] once per frame.
pub fn update_system(mut events: ResMut<Self>) {
events.update();
pub fn update_system(mut events: ResMut<Self>, settings: Option<Res<EventSettings<E>>>) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if let Some(settings) = settings {
events.update(&settings);
} else {
events.update(&Default::default());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}

#[inline]
Expand Down Expand Up @@ -598,7 +648,7 @@ mod tests {
"reader_a receives next unread event"
);

events.update();
events.update(&Default::default());

let mut reader_d = events.get_reader();

Expand All @@ -620,7 +670,7 @@ mod tests {
"reader_d receives all events created before and after update"
);

events.update();
events.update(&Default::default());

assert_eq!(
get_events(&events, &mut reader_missed),
Expand Down Expand Up @@ -654,7 +704,7 @@ mod tests {
assert!(reader.iter(&events).next().is_none());

events.send(E(2));
events.update();
events.update(&Default::default());
events.send(E(3));

assert!(reader.iter(&events).eq([E(2), E(3)].iter()));
Expand Down Expand Up @@ -691,12 +741,12 @@ mod tests {
events.send(TestEvent { i: 0 });
assert!(!events.is_empty());

events.update();
events.update(&Default::default());
assert!(!events.is_empty());

// events are only empty after the second call to update
// due to double buffering.
events.update();
events.update(&Default::default());
assert!(events.is_empty());
}

Expand Down Expand Up @@ -750,12 +800,12 @@ mod tests {
events.send(TestEvent { i: 0 });
let reader = events.get_reader();
assert_eq!(reader.len(&events), 2);
events.update();
events.update(&Default::default());
events.send(TestEvent { i: 0 });
assert_eq!(reader.len(&events), 3);
events.update();
events.update(&Default::default());
assert_eq!(reader.len(&events), 1);
events.update();
events.update(&Default::default());
assert!(reader.is_empty(&events));
}

Expand Down