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

Optimize Event Updates #12936

Merged
merged 19 commits into from
Apr 13, 2024
Merged

Conversation

james7132
Copy link
Member

@james7132 james7132 commented Apr 12, 2024

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 guaranteed O(1) by just checking if a resource's value, swapping two Vecs, and then clearing one of them, the actual cost of running event_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 type event_update_system with a singular exclusive system uses the EventRegistry to update all events instead. Update SubApp::add_event to use EventRegistry instead.

Performance

This speeds reduces the cost of the 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 launching time_system, which should be mostly eliminated with #12869.
image

The actual event_update_system is usually very short, using only a few microseconds on average.
image


Changelog

TODO

Migration Guide

TODO

@james7132 james7132 added A-ECS Entities, components, systems, and events C-Performance A change motivated by improving speed, memory usage or compile times M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Apr 12, 2024
@james7132 james7132 requested review from hymm and maniwani April 12, 2024 04:07
Copy link
Contributor

@jdm jdm left a 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.

crates/bevy_ecs/src/event.rs Outdated Show resolved Hide resolved
mut virt: ResMut<Time<Virtual>>,
real: Res<Time<Real>>,
) {
pub fn update_virtual_time(current: &mut Time, virt: &mut Time<Virtual>, real: &Time<Real>) {
Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Member

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.

@james7132
Copy link
Member Author

Got the actual system to be about 2x faster on average by caching the ComponentId and using untyped resource fetches and bypassing change detection:
image

@alice-i-cecile alice-i-cecile added the M-Needs-Release-Note Work that should be called out in the blog due to impact label Apr 12, 2024
@alice-i-cecile alice-i-cecile added this to the 0.14 milestone Apr 12, 2024
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>() {
Copy link
Member

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?

Copy link
Member Author

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.

@james7132
Copy link
Member Author

Got the actual system to be nearly free by lifting the check out of the function pointer and using change detection.

image

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Apr 13, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Apr 13, 2024
Merged via the queue into bevyengine:main with commit ae9775c Apr 13, 2024
28 checks passed
Shatur added a commit to projectharmonia/bevy_replicon that referenced this pull request May 22, 2024
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.
Shatur added a commit to projectharmonia/bevy_replicon that referenced this pull request May 22, 2024
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.
Shatur added a commit to projectharmonia/bevy_replicon that referenced this pull request May 23, 2024
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]>
@alice-i-cecile
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Performance A change motivated by improving speed, memory usage or compile times M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide M-Needs-Release-Note Work that should be called out in the blog due to impact S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants