Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@jiahaog
Copy link
Member

@jiahaog jiahaog commented Jun 29, 2022

The Handler created through new Handler(Looper.getMainLooper()) is subject to synchronization barriers like vsync. This means that posting tasks to it can result in scheduling delays of up to 16ms. Empirically I have noticed this happening only before the first frame has been drawn, but not sure if it might happen on other occasions.

This is a common pattern for platform message handlers on the main thread, and the delay can stack up and affect startup latency when apps do something like:

void main() async {
  final a = await sendPlatformMessage();
  final b = await sendPlatformMessageWithA(a);
  final c = await sendPlatformMessageWithB(b);

  runApp(MyApp(c));
}

Handler.createAsync helps to avoid this, but this API is only available on SDK >=28. Ideally we would use androidx.core.os.HandlerCompat but we don't have a dependency on it so I opted for a small reimplementation. It differs slightly in that we don't do the reflection fallback to keep things simple.

Tested: Internal global presubmit. I did not add new tests since this is an implementation change and should be covered by existing tests.

This also improves the startup latency for customer:money by 3%. See b/237035671 for more details.

Related Issues

b/237035671
Might be related to flutter/flutter#81559 (comment)

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides].
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See [testing the engine] for instructions on
    writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the [CLA].
  • All existing and new tests are passing.

@jiahaog jiahaog marked this pull request as ready for review June 29, 2022 08:25
@jiahaog jiahaog requested a review from dnfield June 29, 2022 08:25
@jiahaog
Copy link
Member Author

jiahaog commented Jun 29, 2022

Dan, can you take a look since @gaaclarke is out?

@dnfield
Copy link
Contributor

dnfield commented Jun 29, 2022

Test?

@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 (don't just cc him here, he won't see it! He's on Discord!).

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.

@jiahaog
Copy link
Member Author

jiahaog commented Jun 29, 2022

This is an implementation detail covered by existing tests. Not sure how to add a test for this in a simple manner, would you be able to advise?

@dnfield
Copy link
Contributor

dnfield commented Jun 29, 2022

I guess you could test that a handler created with the createAsync compat layer makes isAsynchronous true on messages it sends?

@jiahaog
Copy link
Member Author

jiahaog commented Jun 30, 2022

Thanks, yes we can add a test for the HandlerCompat. Done 🙂

@gaaclarke
Copy link
Member

FYI I know I've seen this issue with customer: money and it helps with their benchmarks but I just checked and we don't have a microbenchmark that seems captures the benefit. That would be nice to have at some point.

@jiahaog
Copy link
Member Author

jiahaog commented Sep 12, 2022

Going to revert this in #36091 first since it causes flutter/flutter#110867 (comment)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

autosubmit Merge PR when tree becomes green via auto submit App needs tests platform-android

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants