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

fuchsia: Implement WakeUp using zx::timer #29019

Merged
merged 1 commit into from
Oct 6, 2021

Conversation

arbreng
Copy link
Contributor

@arbreng arbreng commented Oct 5, 2021

This PR fixes a performance regression that #26783 introduced, where many empty tasks get posted onto the async::Loop backing an fml::MessageLoop.

A long time ago, when gw280 was working on the precursor to #26783, he ran into this same dire performance problems due to the use of async::PostDelayedTask() inside of WakeUp(). Each call to WakeUp() would post a new async::Task to drain flutter's task queue, but draining the task queue itself calls WakeUp() repeatedly. This can cause a storm of empty tasks to be posted to the queue, severely bogging down performance.

The correct fix is to manage draining flutter's task queue using an async::Wait on a zx::timer. WakeUp() just manipulates the timer value. This is very similar to how MessageLoop is implemented on other platforms. gw280's original fix contained this code, but he had to abandon it due to other issues that couldn't be solved at the time (stack overflow errors).

When @hjfreyer and I re-landed the PR, we fixed the stack overflow errors, but we did not adjust the implementation of WakeUp(). It continued to rely on async::PostDelayedTask(). This really impacts performance; even idle CPU usage is higher than it would be otherwise.

The performance loss is especially noticeable in situations where a smart_display is under heavy load (ex. 'Set a 10 minute timer'). The extra CPU load is noticeable enough that even "top" can be used to confirm the regression. The load can become so bad in some situations that FIDL and gRPC deadlines get missed, which causes a cascade of downstream failures.

Test: Added to message_loop_unittests
Fixes: https://b.corp.google.com/issues/201906431
Fixes: flutter/flutter#50678

@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Copy link
Contributor

@akbiggs akbiggs left a comment

Choose a reason for hiding this comment

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

This is awesome.

fml/platform/fuchsia/message_loop_fuchsia.cc Outdated Show resolved Hide resolved
@arbreng arbreng requested review from chinmaygarde and removed request for chinmaygarde October 5, 2021 20:41
@arbreng arbreng merged commit 0e87d51 into flutter:master Oct 6, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 6, 2021
fluttergithubbot pushed a commit to flutter/flutter that referenced this pull request Oct 6, 2021
dnfield pushed a commit to dnfield/engine that referenced this pull request Oct 6, 2021
@arbreng arbreng deleted the fuchsia-message-loop branch October 20, 2021 16:51
clocksmith pushed a commit to clocksmith/flutter that referenced this pull request Oct 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fuchsia MessageLoop needs to re-arm a timer instead of queue a new task on WakeUp()
3 participants