Skip to content

Commit

Permalink
Fix perf degradation on web builds (#11227)
Browse files Browse the repository at this point in the history
# Objective

- Since #10702, the way bevy updates the window leads to major slowdowns
as seen in
    - #11122 
    - #11220
- Slow is bad, furthermore, _very_ slow is _very_ bad. We should fix
this issue.

## Solution

- Move the app update code into the `Event::WindowEvent { event:
WindowEvent::RedrawRequested }` branch of the event loop.
- Run `window.request_redraw()` When `runner_state.redraw_requested`
- Instead of swapping `ControlFlow` between `Poll` and `Wait`, we always
keep it at `Wait`, and use `window.request_redraw()` to schedule an
immediate call to the event loop.
- `runner_state.redraw_requested` is set to `true` when
`UpdateMode::Continuous` and when a `RequestRedraw` event is received.
- Extract the redraw code into a separate function, because otherwise
I'd go crazy with the indentation level.
- Fix #11122.

## Testing

I tested the WASM builds as follow:

```sh
cargo run -p build-wasm-example -- --api webgl2 bevymark
python -m http.server --directory examples/wasm/ 8080
# Open browser at http://localhost:8080
```

On main, even spawning a couple sprites is super choppy. Even if it says
"300 FPS". While on this branch, it is smooth as butter.

I also found that it fixes all choppiness on window resize (tested on
Linux/X11). This was another issue from #10702 IIRC.

So here is what I tested:

- On `wasm`: `many_foxes` and `bevymark`, with `argh::from_env()`
commented out, otherwise we get a cryptic error.
- Both with `PresentMode::AutoVsync` and `PresentMode::AutoNoVsync`
  - On main, it is consistently choppy.
- With this PR, the visible frame rate is consistent with the diagnostic
numbers
- On native (linux/x11) I ran similar tests, making sure that
`AutoVsync` limits to monitor framerate, and `AutoNoVsync` doesn't.

## Future work

Code could be improved, I wanted a quick solution easy to review, but we
really need to make the code more accessible.

- #9768
- ~~**`WinitSettings::desktop_app()` is completely borked.**~~ actually
broken on main as well

### Review guide

Consider enable the non-whitespace diff to see the _real_ change set.
  • Loading branch information
nicopap authored Jan 6, 2024
1 parent a35a151 commit 79021c7
Show file tree
Hide file tree
Showing 3 changed files with 150 additions and 106 deletions.
4 changes: 4 additions & 0 deletions crates/bevy_render/src/renderer/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,10 @@ pub fn render_system(world: &mut World) {
for window in windows.values_mut() {
if let Some(wrapped_texture) = window.swap_chain_texture.take() {
if let Some(surface_texture) = wrapped_texture.try_unwrap() {
// TODO(clean): winit docs recommends calling pre_present_notify before this.
// though `present()` doesn't present the frame, it schedules it to be presented
// by wgpu.
// https://docs.rs/winit/0.29.9/wasm32-unknown-unknown/winit/window/struct.Window.html#method.pre_present_notify
surface_texture.present();
}
}
Expand Down
248 changes: 144 additions & 104 deletions crates/bevy_winit/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,7 @@ pub fn winit_runner(mut app: App) {
}
}
}
runner_state.redraw_requested = false;

match event {
Event::NewEvents(start_cause) => match start_cause {
Expand Down Expand Up @@ -412,7 +413,7 @@ pub fn winit_runner(mut app: App) {
return;
};

let Ok((mut window, mut cache)) = windows.get_mut(window_entity) else {
let Ok((mut window, _)) = windows.get_mut(window_entity) else {
warn!(
"Window {:?} is missing `Window` component, skipping event {:?}",
window_entity, event
Expand Down Expand Up @@ -655,17 +656,33 @@ pub fn winit_runner(mut app: App) {
window: window_entity,
});
}
WindowEvent::RedrawRequested => {
runner_state.redraw_requested = false;
run_app_update_if_should(
&mut runner_state,
&mut app,
&mut focused_windows_state,
event_loop,
&mut create_window_system_state,
&mut app_exit_event_reader,
&mut redraw_event_reader,
);
}
_ => {}
}

if window.is_changed() {
cache.window = window.clone();
let mut windows = app.world.query::<(&mut Window, &mut CachedWindow)>();
if let Ok((window, mut cache)) = windows.get_mut(&mut app.world, window_entity) {
if window.is_changed() {
cache.window = window.clone();
}
}
}
Event::DeviceEvent {
event: DeviceEvent::MouseMotion { delta: (x, y) },
..
} => {
runner_state.redraw_requested = true;
let (mut event_writers, ..) = event_writer_system_state.get_mut(&mut app.world);
event_writers.mouse_motion.send(MouseMotion {
delta: Vec2::new(x as f32, y as f32),
Expand Down Expand Up @@ -726,119 +743,142 @@ pub fn winit_runner(mut app: App) {

app.world.entity_mut(entity).insert(wrapper);
}
event_loop.set_control_flow(ControlFlow::Poll);
event_loop.set_control_flow(ControlFlow::Wait);
}
}
Event::AboutToWait => {
if runner_state.active.should_run() {
if runner_state.active == ActiveState::WillSuspend {
runner_state.active = ActiveState::Suspended;
#[cfg(target_os = "android")]
{
// Remove the `RawHandleWrapper` from the primary window.
// This will trigger the surface destruction.
let mut query =
app.world.query_filtered::<Entity, With<PrimaryWindow>>();
let entity = query.single(&app.world);
app.world.entity_mut(entity).remove::<RawHandleWrapper>();
event_loop.set_control_flow(ControlFlow::Wait);
}
}
let (config, windows) = focused_windows_state.get(&app.world);
let focused = windows.iter().any(|window| window.focused);
let should_update = match config.update_mode(focused) {
UpdateMode::Continuous | UpdateMode::Reactive { .. } => {
// `Reactive`: In order for `event_handler` to have been called, either
// we received a window or raw input event, the `wait` elapsed, or a
// redraw was requested (by the app or the OS). There are no other
// conditions, so we can just return `true` here.
true
}
UpdateMode::ReactiveLowPower { .. } => {
runner_state.wait_elapsed
|| runner_state.redraw_requested
|| runner_state.window_event_received
}
};

if app.plugins_state() == PluginsState::Cleaned && should_update {
// reset these on each update
runner_state.wait_elapsed = false;
runner_state.window_event_received = false;
runner_state.redraw_requested = false;
runner_state.last_update = Instant::now();

app.update();
_ => (),
}
if runner_state.redraw_requested {
let (_, winit_windows, _, _) = event_writer_system_state.get_mut(&mut app.world);
for window in winit_windows.windows.values() {
window.request_redraw();
}
}
};

// decide when to run the next update
let (config, windows) = focused_windows_state.get(&app.world);
let focused = windows.iter().any(|window| window.focused);
match config.update_mode(focused) {
UpdateMode::Continuous => {
event_loop.set_control_flow(ControlFlow::Poll);
}
UpdateMode::Reactive { wait }
| UpdateMode::ReactiveLowPower { wait } => {
if let Some(next) = runner_state.last_update.checked_add(*wait) {
runner_state.scheduled_update = Some(next);
event_loop.set_control_flow(ControlFlow::WaitUntil(next));
} else {
runner_state.scheduled_update = None;
event_loop.set_control_flow(ControlFlow::Wait);
}
}
}
trace!("starting winit event loop");
// TODO(clean): the winit docs mention using `spawn` instead of `run` on WASM.
if let Err(err) = event_loop.run(event_handler) {
error!("winit event loop returned an error: {err}");
}
}

if let Some(app_redraw_events) =
app.world.get_resource::<Events<RequestRedraw>>()
{
if redraw_event_reader.read(app_redraw_events).last().is_some() {
runner_state.redraw_requested = true;
event_loop.set_control_flow(ControlFlow::Poll);
}
}
fn run_app_update_if_should(
runner_state: &mut WinitAppRunnerState,
app: &mut App,
focused_windows_state: &mut SystemState<(Res<WinitSettings>, Query<&Window>)>,
event_loop: &EventLoopWindowTarget<()>,
create_window_system_state: &mut SystemState<(
Commands,
Query<(Entity, &mut Window), Added<Window>>,
EventWriter<WindowCreated>,
NonSendMut<WinitWindows>,
NonSendMut<AccessKitAdapters>,
ResMut<WinitActionHandlers>,
ResMut<AccessibilityRequested>,
)>,
app_exit_event_reader: &mut ManualEventReader<AppExit>,
redraw_event_reader: &mut ManualEventReader<RequestRedraw>,
) {
if !runner_state.active.should_run() {
return;
}
if runner_state.active == ActiveState::WillSuspend {
runner_state.active = ActiveState::Suspended;
#[cfg(target_os = "android")]
{
// Remove the `RawHandleWrapper` from the primary window.
// This will trigger the surface destruction.
let mut query = app.world.query_filtered::<Entity, With<PrimaryWindow>>();
let entity = query.single(&app.world);
app.world.entity_mut(entity).remove::<RawHandleWrapper>();
event_loop.set_control_flow(ControlFlow::Wait);
}
}
let (config, windows) = focused_windows_state.get(&app.world);
let focused = windows.iter().any(|window| window.focused);
let should_update = match config.update_mode(focused) {
// `Reactive`: In order for `event_handler` to have been called, either
// we received a window or raw input event, the `wait` elapsed, or a
// redraw was requested (by the app or the OS). There are no other
// conditions, so we can just return `true` here.
UpdateMode::Continuous | UpdateMode::Reactive { .. } => true,
// TODO(bug): This is currently always true since we only run this function
// if we received a `RequestRedraw` event.
UpdateMode::ReactiveLowPower { .. } => {
runner_state.wait_elapsed
|| runner_state.redraw_requested
|| runner_state.window_event_received
}
};

if let Some(app_exit_events) = app.world.get_resource::<Events<AppExit>>() {
if app_exit_event_reader.read(app_exit_events).last().is_some() {
event_loop.exit();
}
}
}
if app.plugins_state() == PluginsState::Cleaned && should_update {
// reset these on each update
runner_state.wait_elapsed = false;
runner_state.last_update = Instant::now();

// create any new windows
// (even if app did not update, some may have been created by plugin setup)
let (
commands,
mut windows,
event_writer,
winit_windows,
adapters,
handlers,
accessibility_requested,
) = create_window_system_state.get_mut(&mut app.world);

create_windows(
event_loop,
commands,
windows.iter_mut(),
event_writer,
winit_windows,
adapters,
handlers,
accessibility_requested,
);
app.update();

create_window_system_state.apply(&mut app.world);
// decide when to run the next update
let (config, windows) = focused_windows_state.get(&app.world);
let focused = windows.iter().any(|window| window.focused);
match config.update_mode(focused) {
UpdateMode::Continuous => {
runner_state.redraw_requested = true;
}
UpdateMode::Reactive { wait } | UpdateMode::ReactiveLowPower { wait } => {
// TODO(bug): this is unexpected behavior.
// When Reactive, user expects bevy to actually wait that amount of time,
// and not potentially infinitely depending on plateform specifics (which this does)
// Need to verify the plateform specifics (whether this can occur in
// rare-but-possible cases) and replace this with a panic or a log warn!
if let Some(next) = runner_state.last_update.checked_add(*wait) {
runner_state.scheduled_update = Some(next);
event_loop.set_control_flow(ControlFlow::WaitUntil(next));
} else {
runner_state.scheduled_update = None;
event_loop.set_control_flow(ControlFlow::Wait);
}
}
_ => (),
}
};

trace!("starting winit event loop");
if let Err(err) = event_loop.run(event_handler) {
error!("winit event loop returned an error: {err}");
if let Some(app_redraw_events) = app.world.get_resource::<Events<RequestRedraw>>() {
if redraw_event_reader.read(app_redraw_events).last().is_some() {
runner_state.redraw_requested = true;
}
}

if let Some(app_exit_events) = app.world.get_resource::<Events<AppExit>>() {
if app_exit_event_reader.read(app_exit_events).last().is_some() {
event_loop.exit();
}
}
}

// create any new windows
// (even if app did not update, some may have been created by plugin setup)
let (
commands,
mut windows,
event_writer,
winit_windows,
adapters,
handlers,
accessibility_requested,
) = create_window_system_state.get_mut(&mut app.world);

create_windows(
event_loop,
commands,
windows.iter_mut(),
event_writer,
winit_windows,
adapters,
handlers,
accessibility_requested,
);

create_window_system_state.apply(&mut app.world);
}

fn react_to_resize(
Expand Down
4 changes: 2 additions & 2 deletions crates/bevy_winit/src/winit_config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ pub enum UpdateMode {
/// - new [window](`winit::event::WindowEvent`) or [raw input](`winit::event::DeviceEvent`)
/// events have appeared
Reactive {
/// The minimum time from the start of one update to the next.
/// The approximate time from the start of one update to the next.
///
/// **Note:** This has no upper limit.
/// The [`App`](bevy_app::App) will wait indefinitely if you set this to [`Duration::MAX`].
Expand All @@ -93,7 +93,7 @@ pub enum UpdateMode {
/// Use this mode if, for example, you only want your app to update when the mouse cursor is
/// moving over a window, not just moving in general. This can greatly reduce power consumption.
ReactiveLowPower {
/// The minimum time from the start of one update to the next.
/// The approximate time from the start of one update to the next.
///
/// **Note:** This has no upper limit.
/// The [`App`](bevy_app::App) will wait indefinitely if you set this to [`Duration::MAX`].
Expand Down

0 comments on commit 79021c7

Please sign in to comment.