migrate flutter runner to present2 #14162

farchond · 2019-12-06T21:59:48Z

No change in behavior expected. We have 2 frames in flight as before.

By switching to Present2 and specifying a kMaxFramesInFlight however, we
allow us greater flexibility to change how Flutter schedules which will be
changed in the coming CLs.

shell/platform/fuchsia/flutter/session_connection.cc

shell/platform/fuchsia/flutter/vsync_recorder.cc

iskakaushik · 2019-12-09T22:34:55Z

I have only glanced at this PR, noticed that it doesn't have any tests. I realize that the existing implementation is untested, but our current infra should support testing this change. Let me know if you need support writing them.

amott9 · 2019-12-09T23:51:39Z

shell/platform/fuchsia/flutter/session_connection.cc

+        // Since this event is fired on a per-presentation not per-submission
+        // basis, we may have had multiple Present2() calls all end here.


This is a bit confusing. How about something along the the lines:
// Frame presented: update presents in flight with the remaining unhandled present requests.

Made it look more like yours, PTAL.

amott9 · 2019-12-09T23:56:34Z

shell/platform/fuchsia/flutter/session_connection.cc

@@ -92,6 +128,12 @@ void SessionConnection::EnqueueClearOps() {

 void SessionConnection::PresentSession() {


+1 for adding tests on when a present should / should not be called.

shell/platform/fuchsia/flutter/vsync_recorder.cc

shell/platform/fuchsia/flutter/session_connection.h

iskakaushik · 2019-12-10T01:07:46Z

Caught up with @farchond , we need to account for the following scenario. Flutter uses the presentation_time as the scheduling mechanism in VsyncWaiter. I'm worried about the following scenario:

have 2 frames in flight. We get back 2 future times back . Lets say (16 ms, 32 ms).
we update VsyncRecorder to 16 as it is the smallest time in the future.
Vsync recorder would never record 32 as a vsync time as during the next present call we would presumably get 48, ..

We agreed on a couple of ways to work around this problem.

shell/platform/fuchsia/flutter/vsync_recorder.cc

gw280

Really good to see this!

One overall comment is whether we really need FML_CHECK for all these cases, or if we can get away with FML_DCHECK instead.

gw280 · 2020-01-21T19:55:42Z

shell/platform/fuchsia/flutter/BUILD.gn

@@ -452,3 +453,134 @@ fuchsia_archive("flutter_runner_tzdata_tests") {
    },
  ]
 }
+
+executable("flutter_runner_scenic_unittests") {


can we just make these tests part of the flutter_runner_tests?

IIRC, because of dependencies on session_connection, Kaushik and I made flutter_runner_scenic_tests a separate target as it was not possible to do so otherwise.

gw280 · 2020-01-21T19:56:14Z

shell/platform/fuchsia/flutter/meta/flutter_runner_scenic_tests.cmx

@@ -0,0 +1,36 @@
+{


similarly, we can just add these services and features to the existing flutter_runner_tests cmx

Ditto above comment, we can discuss there if necessary.

gw280 · 2020-01-21T20:42:06Z

shell/platform/fuchsia/flutter/session_connection.cc

+  session_wrapper_.set_on_frame_presented_handler(
+      [this, handle = vsync_event_handle_](
+          fuchsia::scenic::scheduling::FramePresentedInfo info) {
+        FML_LOG(INFO) << "OnFramePresented";


Can we TRACE_EVENT here instead?

This was for debugging - removed.

gw280 · 2020-01-21T20:43:41Z

shell/platform/fuchsia/flutter/session_connection.cc

+        // A frame was presented: Update our |frames_in_flight| to match the
+        // updated unfinalized present requests.
+        frames_in_flight_ -= num_presents_handled;
+        FML_CHECK(frames_in_flight_ >= 0);


can we get away with only checking this in debug builds, or is this a catastrophic condition?

Made into a DCHECK.

gw280 · 2020-01-21T21:35:05Z

shell/platform/fuchsia/flutter/session_connection.cc

+        frames_in_flight_allowed_ = info.remaining_presents_in_flight_allowed;
+
+        // If Scenic alloted us 0 frames to begin with, we should fail here.
+        FML_CHECK(frames_in_flight_allowed_ > 0);


do we have to actually abort here or can we just early return and not present?

If there is such an error at startup, I thought it'd be best to fail immediately. If we can't even Present() at the beginning, then there won't be a time when we can.

gw280 · 2020-01-21T21:39:40Z

shell/platform/fuchsia/flutter/BUILD.gn

+    "isolate_configurator.h",
+    "logging.h",
+    "loop.cc",
+    "loop.h",


loop, task_observers, task_runner_adapter etc are all gone in master

gw280 · 2020-01-21T21:41:12Z

shell/platform/fuchsia/flutter/tests/session_connection_unittests.cc

+  async::Loop loop(&kAsyncLoopConfigAttachToCurrentThread);
+  thrd_t fidl_thread;
+  ZX_ASSERT(ZX_OK ==
+            loop.StartThread("SessionConnectionTestThread", &fidl_thread));


I don't see anywhere that we use this thread? Also, can we use FML's Thread classes instead?

I added a comment explaining this in the code: basically we need an async_dispatcher in order to have a ComponentContext. Using a Fuchsia async::Loop gives us one.

gw280 · 2020-01-21T21:43:05Z

shell/platform/fuchsia/flutter/text.txt

+# Flutter testing.
+alias fltestbuild='autoninja -C $FLUTTER_DIR/src/out/fuchsia_release_arm64 flutter_runner_tests-0.far'
+alias fltestpub='$FLUTTER_DIR/src/fuchsia/sdk/linux/tools/pm publish -a -r $FUCHSIA_DIR/out/astro/amber-files/ -f $FLUTTER_DIR/src/out/fuchsia_release_arm64/flutter_runner_tests-0.far'
+alias fltestrun='fx shell run fuchsia-pkg://fuchsia.com/flutter_runner_tests#meta/flutter_runner_tests.cmx'


is this file supposed to be in this PR?

gw280 · 2020-01-21T21:43:18Z

shell/platform/fuchsia/flutter/tests/vsync_recorder_unittests.cc

+#include <lib/sys/cpp/component_context.h>
+
+//#include <fuchsia/sys/component_context/cpp/fidl.h>
+//#include <fuchsia/sys/index/cpp/fidl.h>


nit: commented out lines of code

gw280 · 2020-01-21T21:45:00Z

shell/platform/fuchsia/flutter/tests/vsync_recorder_unittests.cc

+}
+
+// IMPORTANT NOTE: Because there only exists one VsyncRecorder, the order of
+// these tests matter.


can we isolate each test using SetUp() and TearDown()?

arbreng · 2020-01-23T01:47:41Z

shell/platform/fuchsia/flutter/tests/session_connection_unittests.cc

+  // These InterfacePtrs can be initialized any time after |context_|, but must
+  // be member variables so they remain in scope for the duration of the test.
+  fidl::InterfacePtr<fuchsia::ui::scenic::Scenic> scenic_ =
+      context_->svc()->Connect<fuchsia::ui::scenic::Scenic>();


Can we initialize these in SetUp? I was confused until I saw them here

arbreng · 2020-01-23T01:51:40Z

shell/platform/fuchsia/flutter/tests/session_connection_unittests.cc

+
+namespace flutter_runner_test {
+
+class SessionConnectionTest : public ::testing::Test {


Is this hermetic? We are using the actual system Scenic here. Let's file an issue to track making this hermetic via TestWithEnvironment (for the hackathon)

Filed issue fxb/44542

googlebot · 2020-01-23T17:29:39Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

googlebot · 2020-01-23T17:29:52Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

googlebot · 2020-01-23T17:44:13Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

googlebot · 2020-01-23T17:44:13Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

No change in behavior expected. We have 2 frames in flight as before. By switching to Present2 and specifying a kMaxFramesInFlight however, we allow us greater flexibility to change how Flutter schedules. This change also adds tests for SessionConnection and VsyncRecorder.

iskakaushik

LGTM

…ine#14162)

* 9347c93 Roll src/third_party/dart c8ed304e979a..3414b5167554 (52 commits) (flutter/engine#16362) * 16cd6f0 Roll fuchsia/sdk/core/mac-amd64 from 6h3IH... to Ke00Y... (flutter/engine#16360) * 8c6cc65 Fix runtime type errors when running with canvaskit (flutter/engine#16312) * 677b563 Refactor of Vulkan GPUSurface code (flutter/engine#16224) * 7ca44d3 Kill the test harness if any test exceeds a timeout. (flutter/engine#16349) * 7f6149c Revert "Remove use of the deprecated AccessibilityNodeInfo boundsInPa… (flutter/engine#16355) * 488f489 Roll fuchsia/sdk/core/linux-amd64 from Tszo5... to VJv0H... (flutter/engine#16363) * 7c9b11a Roll src/third_party/skia 71ce449d2814..2aee7d24da8f (5 commits) (flutter/engine#16364) * 7e1d144 Expose enable-service-port-fallback switch (flutter/engine#16366) * 4cda916 Expose the dart kernel snapshot target and copied assets as a public dependency (flutter/engine#16266) * 73c5130 Roll src/third_party/dart 3414b5167554..68e904e444dc (17 commits) (flutter/engine#16367) * 1cd8f3b Fix and consolidate wstring conversion utils (flutter/engine#16342) * b98a39e Add docs (flutter/engine#16368) * e3e6de2 Roll buildroot to c44791c89d2b51bfce864ab2cf5228d41ece1fcc (flutter/engine#16371) * e24ec59 Fuchsia a11y actions (flutter/engine#16321) * d8400c9 Roll src/third_party/skia 2aee7d24da8f..14d64afaa8a3 (10 commits) (flutter/engine#16374) * eeabd4d Roll src/third_party/dart 68e904e444dc..48808f7dce81 (17 commits) (flutter/engine#16377) * 22b994c Roll fuchsia/sdk/core/mac-amd64 from Ke00Y... to ubThi... (flutter/engine#16378) * 0471f44 Roll src/third_party/skia 14d64afaa8a3..6c9b1fd6663e (7 commits) (flutter/engine#16380) * 852d824 Roll src/third_party/dart 48808f7dce81..34893faa6079 (7 commits) (flutter/engine#16381) * 4aa2083 Roll src/third_party/skia 6c9b1fd6663e..bc3307c395e2 (1 commits) (flutter/engine#16383) * 036c370 Copied Apple's semantics for switches, made checkboxes the same. (flutter/engine#16211) * c107969 fix build_id logic for fuchsia symbols (flutter/engine#16397) * 11b7704 [fuchsia] Migrate flutter runner to use Present2 (flutter/engine#14162) * 646ec35 Update license output (flutter/engine#16379) * 925c60b Fix Windows embedding issues caught by clang (flutter/engine#16369) * 31bf3e1 Roll src/third_party/dart 34893faa6079..b3396cbdcae1 (36 commits) (flutter/engine#16422) * 8f89bac [web] Fixes incorrect transform when context save and transforms are deferred. (flutter/engine#16412) * f34bc65 use percent for golden diff rates; tighten the values (flutter/engine#16430)

No change in behavior expected. We have 2 frames in flight as before. By switching to Present2 and specifying a kMaxFramesInFlight however, we allow us greater flexibility to change how Flutter schedules its frames. This change also adds tests for SessionConnection and VsyncRecorder.

…)" This reverts commit 4cd9a53.

googlebot added the cla: yes label Dec 6, 2019

auto-assign bot requested a review from GaryQian December 6, 2019 21:59

farchond changed the title ~~migrate to flutter runner to present2~~ migrate flutter runner to present2 Dec 6, 2019

liyuqian requested review from nathanrogersgoogle and amott9 December 6, 2019 22:17

cbracken requested a review from iskakaushik December 9, 2019 19:13

farchond requested a review from liyuqian December 9, 2019 21:31

mikaelpessa reviewed Dec 9, 2019

View reviewed changes

amott9 reviewed Dec 9, 2019

View reviewed changes

iskakaushik reviewed Dec 10, 2019

View reviewed changes

liyuqian reviewed Dec 10, 2019

View reviewed changes

shell/platform/fuchsia/flutter/vsync_recorder.cc Show resolved Hide resolved

gw280 reviewed Jan 21, 2020

View reviewed changes

farchond requested review from arbreng, gw280 and iskakaushik January 22, 2020 17:24

arbreng approved these changes Jan 23, 2020

View reviewed changes

googlebot added cla: no and removed cla: yes labels Jan 23, 2020

farchond force-pushed the present_migration branch from 60f28a6 to 95d4819 Compare January 23, 2020 17:44

googlebot added cla: yes and removed cla: no labels Jan 23, 2020

farchond force-pushed the present_migration branch 3 times, most recently from 421eb32 to 0cab201 Compare January 28, 2020 00:08

farchond force-pushed the present_migration branch from 0cab201 to ba4a80b Compare January 29, 2020 19:45

farchond force-pushed the present_migration branch 2 times, most recently from 4d724d9 to 2a0e694 Compare February 5, 2020 01:49

add flutter_runner_scenic_tests to test infra

e969973

farchond force-pushed the present_migration branch from 2a0e694 to e969973 Compare February 5, 2020 02:26

iskakaushik approved these changes Feb 5, 2020

View reviewed changes

iskakaushik merged commit 11b7704 into flutter:master Feb 5, 2020

engine-flutter-autoroll mentioned this pull request Feb 5, 2020

Roll engine 81dffd1241e6..11b770489f1e (24 commits) flutter/flutter#50227

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 5, 2020

11b7704 [fuchsia] Migrate flutter runner to use Present2 (flutter/eng…

d83452e

…ine#14162)

engine-flutter-autoroll mentioned this pull request Feb 6, 2020

Roll engine 81dffd1241e6..925c60b38bad (26 commits) flutter/flutter#50238

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 6, 2020

11b7704 [fuchsia] Migrate flutter runner to use Present2 (flutter/eng…

9701f79

…ine#14162)

engine-flutter-autoroll mentioned this pull request Feb 6, 2020

Roll engine 81dffd1241e6..8f89bac4b785 (28 commits) flutter/flutter#50250

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 6, 2020

11b7704 [fuchsia] Migrate flutter runner to use Present2 (flutter/eng…

aa54b55

…ine#14162)

engine-flutter-autoroll mentioned this pull request Feb 6, 2020

Roll engine 81dffd1241e6..f34bc65bee49 (29 commits) flutter/flutter#50255

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 6, 2020

11b7704 [fuchsia] Migrate flutter runner to use Present2 (flutter/eng…

e74ed11

…ine#14162)

NoamDev added a commit to NoamDev/engine that referenced this pull request Feb 27, 2020

Revert "[fuchsia] Migrate flutter runner to use Present2 (flutter#14162…

d8023c4

…)" This reverts commit 4cd9a53.

		// Since this event is fired on a per-presentation not per-submission
		// basis, we may have had multiple Present2() calls all end here.

		@@ -92,6 +128,12 @@ void SessionConnection::EnqueueClearOps() {

		void SessionConnection::PresentSession() {


		namespace flutter_runner_test {

		class SessionConnectionTest : public ::testing::Test {

migrate flutter runner to present2 #14162

migrate flutter runner to present2 #14162

Conversation

farchond commented Dec 6, 2019

iskakaushik commented Dec 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iskakaushik commented Dec 10, 2019

gw280 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

googlebot commented Jan 23, 2020

googlebot commented Jan 23, 2020

googlebot commented Jan 23, 2020

googlebot commented Jan 23, 2020

iskakaushik left a comment

Choose a reason for hiding this comment