Fix the first frame logic in tracing and driver #35297

liyuqian · 2019-06-28T22:02:17Z

This should fix #31442 and #34867

gaaclarke

I'm not familiar with the profiling code but it LGTM except for my comment.

gaaclarke · 2019-07-03T00:26:18Z

packages/flutter/lib/src/widgets/binding.dart

Is this happening after the frame has been presented or rasterized? The language of this PR is talking about rasterization but what I found out is that what I was really interested in is not when the frame was rasterized but when it was presented, which is a subtle difference.

I'll revise the language to make it clear that:

We're only waiting for rasterization

Usually, the time that a frame is rasterized is very close to the time that it gets presented. Specifically, rasterization is the last expensive phase of a frame that's still in Flutter's control.

liyuqian · 2019-07-08T19:51:34Z

Ping @Hixie and @goderbauer

goderbauer

I don't have all the context to properly review this, maybe @Hixie can also take a look?

goderbauer · 2019-07-09T17:06:57Z

dev/devicelab/manifest.yaml

Why mark so many as flaky? Isn't this just changing the behavior of only the transition test for now?

Although this test doesn't wait for the first frame, the way that we measure the start_up time changes. Hence I marked all start_up tests to be flaky.

goderbauer · 2019-07-09T17:11:54Z

packages/flutter/lib/src/widgets/binding.dart

Is there value in doing all of this in release mode? Or should this be guarded to only profile and debug mode?

I added this to release mode so _firstFrameCompleter and thus WidgetsBinding.firstFrameRasterized is available in the release mode. It might be useful to handle the splash screen during add2app?

liyuqian · 2019-07-11T17:54:22Z

@goderbauer : do my answers solve your concerns?

dnfield

LGTM.

I'm hoping we can pretty quickly determine if this test actually causes flakes or not and either revert or mark as not flaky.

codecov · 2019-07-24T17:49:14Z

Codecov Report

Merging #35297 into master will decrease coverage by 1.07%.
The diff coverage is 16.66%.

@@            Coverage Diff             @@
##           master   #35297      +/-   ##
==========================================
- Coverage   55.31%   54.23%   -1.08%     
==========================================
  Files         190      190              
  Lines       17590    17589       -1     
==========================================
- Hits         9730     9540     -190     
- Misses       7860     8049     +189

Flag	Coverage Δ
#flutter_tool	`54.23% <16.66%> (-1.08%)`	⬇️

Impacted Files	Coverage Δ
packages/flutter_tools/lib/src/tracing.dart	`1.88% <16.66%> (-0.04%)`	⬇️
...lutter_tools/lib/src/build_system/targets/ios.dart	`0% <0%> (-100%)`	⬇️
packages/flutter_tools/lib/src/compile.dart	`31.56% <0%> (-43.53%)`	⬇️
packages/flutter_tools/lib/src/commands/run.dart	`19.13% <0%> (-41.63%)`	⬇️
...ckages/flutter_tools/lib/src/commands/devices.dart	`53.84% <0%> (-23.08%)`	⬇️
...tter_tools/lib/src/build_system/targets/macos.dart	`64.7% <0%> (-11.77%)`	⬇️
packages/flutter_tools/lib/src/run_cold.dart	`0% <0%> (-10.39%)`	⬇️
...ackages/flutter_tools/lib/src/resident_runner.dart	`45.18% <0%> (-8.04%)`	⬇️
packages/flutter_tools/lib/src/base/utils.dart	`87% <0%> (-7.5%)`	⬇️
packages/flutter_tools/lib/src/version.dart	`93.1% <0%> (-2.47%)`	⬇️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aa6384c...e4454fc. Read the comment docs.

liyuqian · 2019-07-24T19:38:30Z

Adding @jonahwilliams as the reviewer as rebasing the PR to the head of master triggers the tool codecov tests.

Ping @goderbauer for the overall review. If there's no objection, I'll land this soon to start measuring our first frame latency correctly.

dnfield · 2019-07-24T22:18:42Z

You can ignore the codecov bot - it's not quite working.

jonahwilliams · 2019-07-24T22:21:30Z

It is correct in that the tools code here is not covered by tests. I think we've decided that its not the responsibility of contributors making small changes to rewrite the tools to be testable

goderbauer

LGTM

This reverts commit 68fc723.

…7027) This reverts commit 68fc723.

tvolkert · 2019-07-27T00:57:01Z

For posterity, this was reverted because it was breaking Flutter devicelab tests

…ter#35297)" (flutter#37027)" This reverts commit 3068fc4.

This should fix flutter#31442 and flutter#34867

…" (flutter#37027) This reverts commit 68fc723.

…7192) This relands #35297 The followings have been done to fix the broken tests: 1. Add `didSendFirstFrameRasterizedEvent` extension and its tests 2. Wait for `didSendFirstFrameRasterizedEvent` instead of `didSendFirstFrameEvent` during start up tests 3. Mark missed (probably newly added) start up tests as flaky

Hixie · 2019-08-25T20:58:02Z

@liyuqian Can we mark the previously existing tests non-flaky again and/or revert this PR? We can't be having these critical benchmarks be considered ok-to-break.

liyuqian · 2019-08-25T21:31:44Z

@Hixie : please see #37192 (comment)

googlebot added the cla: yes label Jun 28, 2019

liyuqian force-pushed the fix_first_frame branch from 7a75868 to 247424c Compare June 28, 2019 22:17

liyuqian requested review from Hixie, gaaclarke and goderbauer July 2, 2019 03:11

gaaclarke reviewed Jul 3, 2019

View reviewed changes

goderbauer reviewed Jul 9, 2019

View reviewed changes

dnfield approved these changes Jul 17, 2019

View reviewed changes

liyuqian added 3 commits July 24, 2019 10:28

Fix the first frame logic in tracing and driver

4a6a3ca

Mark various affected tests as flaky

f71022f

Improve docs on rasterized vs presented.

e4454fc

liyuqian force-pushed the fix_first_frame branch from 499aa82 to e4454fc Compare July 24, 2019 17:30

liyuqian requested a review from jonahwilliams July 24, 2019 19:36

goderbauer approved these changes Jul 25, 2019

View reviewed changes

liyuqian merged commit 68fc723 into flutter:master Jul 26, 2019

jonahwilliams pushed a commit that referenced this pull request Jul 26, 2019

Revert "Fix the first frame logic in tracing and driver (#35297)"

f528d38

This reverts commit 68fc723.

jonahwilliams mentioned this pull request Jul 26, 2019

Revert "Fix the first frame logic in tracing and driver" #37027

Merged

jonahwilliams pushed a commit that referenced this pull request Jul 26, 2019

Revert "Fix the first frame logic in tracing and driver (#35297)" (#3…

3068fc4

…7027) This reverts commit 68fc723.

liyuqian added a commit to liyuqian/flutter that referenced this pull request Jul 29, 2019

Revert "Revert "Fix the first frame logic in tracing and driver (flut…

83fbe92

…ter#35297)" (flutter#37027)" This reverts commit 3068fc4.

liyuqian mentioned this pull request Jul 29, 2019

Reland "Fix the first frame logic in tracing and driver (#35297)" #37192

Merged

johnsonmh pushed a commit to johnsonmh/flutter that referenced this pull request Jul 30, 2019

Fix the first frame logic in tracing and driver (flutter#35297)

44d481a

This should fix flutter#31442 and flutter#34867

johnsonmh pushed a commit to johnsonmh/flutter that referenced this pull request Jul 30, 2019

Revert "Fix the first frame logic in tracing and driver (flutter#35297)…

4b6db5e

…" (flutter#37027) This reverts commit 68fc723.

liyuqian deleted the fix_first_frame branch August 7, 2019 16:58

liyuqian mentioned this pull request Nov 8, 2019

Correct the first frame waiting logic and measurement #34867

Closed

github-actions bot locked as resolved and limited conversation to collaborators Aug 4, 2021

Fix the first frame logic in tracing and driver #35297

Fix the first frame logic in tracing and driver #35297

Uh oh!

Conversation

liyuqian commented Jun 28, 2019

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

gaaclarke Jul 3, 2019

Choose a reason for hiding this comment

Uh oh!

liyuqian Jul 3, 2019

Choose a reason for hiding this comment

Uh oh!

liyuqian commented Jul 8, 2019

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

goderbauer Jul 9, 2019

Choose a reason for hiding this comment

Uh oh!

liyuqian Jul 9, 2019

Choose a reason for hiding this comment

Uh oh!

goderbauer Jul 9, 2019

Choose a reason for hiding this comment

Uh oh!

liyuqian Jul 9, 2019

Choose a reason for hiding this comment

Uh oh!

liyuqian commented Jul 11, 2019

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jul 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

liyuqian commented Jul 24, 2019

Uh oh!

dnfield commented Jul 24, 2019

Uh oh!

jonahwilliams commented Jul 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

tvolkert commented Jul 27, 2019

Uh oh!

Hixie commented Aug 25, 2019

Uh oh!

liyuqian commented Aug 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

codecov bot commented Jul 24, 2019 •

edited

Loading

jonahwilliams commented Jul 24, 2019 •

edited

Loading