Reland "Fix the first frame logic in tracing and driver (#35297)" by liyuqian · Pull Request #37192 · flutter/flutter

liyuqian · 2019-07-29T20:10:39Z

This relands #35297

The followings have been done to fix the broken tests:

Add didSendFirstFrameRasterizedEvent extension and its tests
Wait for didSendFirstFrameRasterizedEvent instead of
didSendFirstFrameEvent during start up tests
Mark missed (probably newly added) start up tests as flaky

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

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

jonahwilliams

LGTM

codecov · 2019-07-29T20:36:51Z

Codecov Report

Merging #37192 into master will increase coverage by 0.61%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master   #37192      +/-   ##
==========================================
+ Coverage   54.55%   55.16%   +0.61%     
==========================================
  Files         193      193              
  Lines       17849    17851       +2     
==========================================
+ Hits         9738     9848     +110     
+ Misses       8111     8003     -108

Flag	Coverage Δ
#flutter_tool	`55.16% <0%> (+0.61%)`	⬆️

Impacted Files	Coverage Δ
packages/flutter_tools/lib/src/vmservice.dart	`37.3% <0%> (+0.15%)`	⬆️
packages/flutter_tools/lib/src/tracing.dart	`0% <0%> (ø)`	⬆️
.../flutter_tools/lib/src/commands/build_fuchsia.dart	`13.63% <0%> (-68.19%)`	⬇️
...tools/lib/src/fuchsia/fuchsia_kernel_compiler.dart	`0% <0%> (-7.7%)`	⬇️
...ages/flutter_tools/lib/src/commands/build_aot.dart	`29.59% <0%> (-6.13%)`	⬇️
...ges/flutter_tools/lib/src/fuchsia/fuchsia_sdk.dart	`64.81% <0%> (-3.71%)`	⬇️
...utter_tools/lib/src/reporting/crash_reporting.dart	`72.5% <0%> (-2.5%)`	⬇️
...lutter_tools/lib/src/android/android_workflow.dart	`62.33% <0%> (-1.95%)`	⬇️
...ages/flutter_tools/lib/src/base/user_messages.dart	`49.03% <0%> (-0.97%)`	⬇️
packages/flutter_tools/lib/src/cache.dart	`41.83% <0%> (-0.77%)`	⬇️
... and 9 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 c8be195...662638e. Read the comment docs.

goderbauer · 2019-07-30T07:45:05Z

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

+  /// A future that completes when the Flutter engine has rasterized the first
+  /// frame.
+  ///
+  /// {@macro flutter.frame_rasterize`_vs_presented}


nit: is the ` supposed to be in there?

No. Thanks!

goderbauer · 2019-07-30T07:46:48Z

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

  /// This value can also be obtained over the VM service protocol as
  /// `ext.flutter.didSendFirstFrameEvent`.
+  ///
+  /// See also [firstFrameRasterized].


style for "see also" is:

Suggested change

/// See also [firstFrameRasterized].

/// See also:

///

/// * [firstFrameRasterized], <reason why I should look there>.

goderbauer · 2019-07-30T07:47:21Z

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

+  /// A future that completes when the Flutter engine has rasterized the first
+  /// frame.
+  ///
+  /// {@macro flutter.frame_rasterize`_vs_presented}


This should probably have a "see also" section (see below for style) linking to firstFrameRasterized.

goderbauer · 2019-07-30T07:47:39Z

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

  ///
  /// {@macro flutter.frame_rasterized_vs_presented}
-  Future<void> get firstFrameRasterized => _firstFrameCompleter.future;
+  bool get firstFrameRasterized => _firstFrameCompleter.isCompleted;


Add a "see also" section linking to the future version?

goderbauer · 2019-07-30T07:49:17Z

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

+  /// frame.
+  ///
+  /// {@macro flutter.frame_rasterize`_vs_presented}
+  Future<void> get waitUntilFirstFrameRasterized => _firstFrameCompleter.future;


The name is a little misleading because calling the getter doesn't actually wait for anything. The caller has to do the waiting by awaiting on the result. Not sure what a better name would be, though...

I can't think of a better name either. But since Dart never has a synchronous waiting mechanism, it seems to be Ok to return a Future for a waitUntilXYZ function? The only way to wait in Dart is await (see, e.g., https://github.com/flutter/flutter/blob/master/packages/flutter_driver/lib/src/extension/extension.dart#L242)?

liyuqian

Thanks for the suggestions!

liyuqian · 2019-07-30T17:40:22Z

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

  ///
  /// {@macro flutter.frame_rasterized_vs_presented}
-  Future<void> get firstFrameRasterized => _firstFrameCompleter.future;
+  bool get firstFrameRasterized => _firstFrameCompleter.isCompleted;


liyuqian · 2019-07-30T17:40:30Z

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

+  /// A future that completes when the Flutter engine has rasterized the first
+  /// frame.
+  ///
+  /// {@macro flutter.frame_rasterize`_vs_presented}


No. Thanks!

liyuqian · 2019-07-30T17:42:16Z

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

+  /// A future that completes when the Flutter engine has rasterized the first
+  /// frame.
+  ///
+  /// {@macro flutter.frame_rasterize`_vs_presented}


liyuqian · 2019-07-30T17:43:19Z

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

  /// This value can also be obtained over the VM service protocol as
  /// `ext.flutter.didSendFirstFrameEvent`.
+  ///
+  /// See also [firstFrameRasterized].


liyuqian · 2019-07-30T17:50:09Z

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

+  /// frame.
+  ///
+  /// {@macro flutter.frame_rasterize`_vs_presented}
+  Future<void> get waitUntilFirstFrameRasterized => _firstFrameCompleter.future;


I can't think of a better name either. But since Dart never has a synchronous waiting mechanism, it seems to be Ok to return a Future for a waitUntilXYZ function? The only way to wait in Dart is await (see, e.g., https://github.com/flutter/flutter/blob/master/packages/flutter_driver/lib/src/extension/extension.dart#L242)?

goderbauer

LGTM

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

@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:29:48Z

@Hixie : This PR is quite old so reverting it may introduce many conflicts. PR #39212 should revert the effect of this PR on that flaky test.

liyuqian added 2 commits July 29, 2019 12:45

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

83fbe92

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

Fix start up tests

dba3159

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

googlebot added the cla: yes label Jul 29, 2019

liyuqian requested review from dnfield, goderbauer and jonahwilliams July 29, 2019 20:12

jonahwilliams approved these changes Jul 29, 2019

View reviewed changes

goderbauer reviewed Jul 30, 2019

View reviewed changes

"See also"

662638e

liyuqian commented Jul 30, 2019

View reviewed changes

Piinks added engine flutter/engine related. See also e: labels. framework flutter/packages/flutter repository. See also f: labels. labels Jul 30, 2019

goderbauer approved these changes Jul 31, 2019

View reviewed changes

liyuqian merged commit e77237d into flutter:master Jul 31, 2019

liyuqian deleted the reland_first_frame branch August 1, 2019 17:20

liyuqian mentioned this pull request Aug 25, 2019

Fix the first frame logic in tracing and driver #35297

Merged

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

-  /// See also [firstFrameRasterized].
+  /// See also:
+  ///
+  ///  * [firstFrameRasterized], <reason why I should look there>.

Conversation

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

Uh oh!

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liyuqian left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

Hixie commented Aug 25, 2019

Uh oh!

liyuqian commented Aug 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

liyuqian commented Jul 29, 2019 •

edited

Loading

codecov bot commented Jul 29, 2019 •

edited

Loading