-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Optimize Event Updates #12936
Optimize Event Updates #12936
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes sense! Systems that scale linearly with events are an easy footgun in the current implementation.
mut virt: ResMut<Time<Virtual>>, | ||
real: Res<Time<Real>>, | ||
) { | ||
pub fn update_virtual_time(current: &mut Time, virt: &mut Time<Virtual>, real: &Time<Real>) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the bevy_time changes supposed to be part of this OR? I don't understand how they're related.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is in a similar vein of being a common cost in First
, being O(1)
systems that run in sequence and thus commonly incurring a system task overhead and a context switch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a moderate preference to move these changes into their own PR, but won't block on it.
Co-authored-by: Josh Matthews <[email protected]>
s.0 = false; | ||
/// A system that calls [`Events::update`] on all registered [`Events`] in the world. | ||
pub fn event_update_system(world: &mut World) { | ||
if world.contains_resource::<EventRegistry>() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this better than just init_resource
on the registry here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If for whatever reason, someone is running without any events in their World, this would avoid stealth allocating it.
Co-authored-by: Alice Cecile <[email protected]>
Events was implemented this way since the initial release of replicon (it was more then a year ago, time flies quickly!), but it have some flaws: - Public API for custom ser/de require user to define the whole sending and receiving functions which is not ergonomic. It's especially bad for server events where user need to use a few public helpers that we provided to reduce the boilerplate. - It creates a new set of systems for each replicon event. It's not very efficient and Bevy recently did [a nice optimization](bevyengine/bevy#12936) which we also can do. It won't be that noticeable since user register much less amount of replicon events, but nice to have.
Events was implemented this way since the initial release of replicon (it was more then a year ago, time flies quickly!), but it have some flaws: - Public API for custom ser/de require user to define the whole sending and receiving functions which is not ergonomic. It's especially bad for server events where user need to use a few public helpers that we provided to reduce the boilerplate. - It creates a new set of systems for each replicon event. It's not very efficient and Bevy recently did [a nice optimization](bevyengine/bevy#12936) which we also can do. It won't be that noticeable since user register much less amount of replicon events, but nice to have. I think it's time to revisit the implementation and fix the mentioned flaws.
Events was implemented this way since the initial release of replicon (it was more then a year ago, time flies quickly!), but it have some flaws: - Public API for custom ser/de require user to define the whole sending and receiving functions which is not ergonomic. It's especially bad for server events where user need to use a few public helpers that we provided to reduce the boilerplate. - It creates a new set of systems for each replicon event. It's not very efficient and Bevy recently did [a nice optimization](bevyengine/bevy#12936) which we also can do. It won't be that noticeable since user register much less amount of replicon events, but nice to have. I think it's time to revisit the implementation and fix the mentioned flaws. Co-authored-by: UkoeHB <[email protected]>
Thank you to everyone involved with the authoring or reviewing of this PR! This work is relatively important and needs release notes! Head over to bevyengine/bevy-website#1299 if you'd like to help out. |
Objective
Improve performance scalability when adding new event types to a Bevy app. Currently, just using Bevy in the default configuration, all apps spend upwards of 100+us in the
First
schedule, every app tick, evaluating if it should update events or not, even if events are not being used for that particular frame, and this scales with the number of Events registered in the app.Solution
As
Events::update
is guaranteedO(1)
by just checking if a resource's value, swapping two Vecs, and then clearing one of them, the actual cost of runningevent_update_system
is very cheap. The overhead of doing system dependency injection, task scheduling ,and the multithreaded executor outweighs the cost of running the system by a large margin.Create an
EventRegistry
resource that keeps a number of function pointers that update each event. Replace the per-event typeevent_update_system
with a singular exclusive system uses theEventRegistry
to update all events instead. UpdateSubApp::add_event
to useEventRegistry
instead.Performance
This speeds reduces the cost of the
![image](https://private-user-images.githubusercontent.com/3137680/321911144-037624be-21a2-4dc2-a42f-9d0bfa3e9b4a.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MzQ0NTQ1ODcsIm5iZiI6MTczNDQ1NDI4NywicGF0aCI6Ii8zMTM3NjgwLzMyMTkxMTE0NC0wMzc2MjRiZS0yMWEyLTRkYzItYTQyZi05ZDBiZmEzZTliNGEucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI0MTIxNyUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNDEyMTdUMTY1MTI3WiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9Y2ZhMjU5ZTFkYTI1ODA3Y2M0ZjhiMmU2ODA0ZDYxYWVkMzUzYTdlNDQ1ZjE1MWE4MzVkYWEyNmM0Y2QyYzhjNCZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QifQ.PfuYVFPJW2Gjs1yg3l601BftdOUiQGswKu6yPxUgrdY)
First
schedule in both many_foxes and many_cubes by over 80%. Note this is with system spans on. The majority of this is now context-switching costs from launchingtime_system
, which should be mostly eliminated with #12869.The actual
![image](https://private-user-images.githubusercontent.com/3137680/322148769-01ff1689-3595-49b6-8f09-5c44bcf903e8.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MzQ0NTQ1ODcsIm5iZiI6MTczNDQ1NDI4NywicGF0aCI6Ii8zMTM3NjgwLzMyMjE0ODc2OS0wMWZmMTY4OS0zNTk1LTQ5YjYtOGYwOS01YzQ0YmNmOTAzZTgucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI0MTIxNyUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNDEyMTdUMTY1MTI3WiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9YmUyMmIzOGU3ZDAyM2RkN2JkZWY2N2E3MGIzY2E0MzBlZTYxYWMwMTY2ODFiOTYzODFlY2RkMTVjYWQ1MTE1MiZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QifQ.C0AlZrE4eed3NBQQdCe2CuZttADq_4NZhpAePrDQTpk)
event_update_system
is usually very short, using only a few microseconds on average.Changelog
TODO
Migration Guide
TODO