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

servoshell: fix lockups while animating #30322

Merged
merged 6 commits into from Sep 12, 2023

Conversation

delan
Copy link
Sponsor Member

@delan delan commented Sep 8, 2023

With the minibrowser enabled, we respond to input events by calling request_redraw, which defers the egui update to a RedrawRequested event. But once we’re there, we run the compositor asynchronously by dispatching Servo a Refresh event, which defers painting (both compositor and egui) and presenting to some later tick of the event loop.

Per rust-windowing/winit#2900, this seems to be incorrect:

applications should redraw any time they get a RedrawRequested callback (and they shouldn't defer the redraw outside of the winit callback)

When there are active animations (CSS animations, CSS transitions, and sometimes rAF), servoshell pumps its event loop as quickly as possible, without waiting for a waker. For reasons that are unclear to me, our misuse of RedrawRequested interacts with active animations to create a situation where:

  • the event loop is flooded with a sequence of waker events (Recomposite → Webrender → ReadyToPresent)
  • every second tick of the event loop requires a present, which blocks for the remaining time until vsync
  • we never reach MainEventsCleared, RedrawEventsCleared, RedrawRequested
  • we never get a chance to handle any input events or window resize events

This patch fixes those lockups by moving all painting and presenting to the RedrawRequested event handler, with two exceptions. When the compositor paints a frame of its own volition (e.g. due to WebRender), we request a redraw but suppress the recomposite step, since it already happened. When we are handling a resize event, we continue to call Servo::repaint_synchronously as before, but now we also update and paint the minibrowser and present the frame.

We also add trace logs for the flow of EmbedderEvent, EmbedderMsg, and winit events, trace logs for the major RedrawRequested and PumpRequest branches, and a debug message when we recomposite without presenting.

To test this manually, run the repros in #30312 and check that you can interact with the minibrowser ui, click on the page (TWGL Tunnel only), and resize the window, for at least 30 seconds.


  • There are tests for these changes OR
  • These changes do not require tests because the minibrowser is not currently testable

ports/servoshell/app.rs Outdated Show resolved Hide resolved
ports/servoshell/app.rs Show resolved Hide resolved
@jdm jdm enabled auto-merge September 11, 2023 11:44
@delan
Copy link
Sponsor Member Author

delan commented Sep 11, 2023

9d5fd40 disables the needs_recomposite optimisation for now, since it causes problems on @atbrakhi’s machine that need deeper investigation (#30331). This shouldn’t affect the correctness of the patch.

@jdm jdm disabled auto-merge September 11, 2023 12:44
@jdm jdm enabled auto-merge September 11, 2023 12:44
@delan delan mentioned this pull request Sep 11, 2023
27 tasks
@delan delan disabled auto-merge September 11, 2023 15:02
@delan delan added this pull request to the merge queue Sep 12, 2023
Merged via the queue into master with commit 1bbd0c1 Sep 12, 2023
10 checks passed
@delan delan deleted the servoshell-fix-lockups-while-animating branch September 12, 2023 04:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

servoshell: minibrowser can cause lockups while animating
2 participants