Add flushUIThreadTasks service RPC #3898

B3rn475 · 2017-07-18T22:00:51Z

In #3833 the _flutter.listViews RPC moved from thread based to lock based synchronization.
The thread based synchronization side effect was used by flutter benchmarks in https://github.com/flutter/flutter/blob/master/packages/flutter_tools/lib/src/vmservice.dart#L1223 and
https://github.com/flutter/flutter/blob/master/packages/flutter_tools/lib/src/run_hot.dart#L156 to ensure the completeness of the restart/reload and so correct timing.

A new RPC _flutter.flushUIThreadTasks is introduced to allow the flutter benchmarks to reintroduce thread based synchronization.

Related flutter/flutter#11241

In #3833 the _flutter.listViews RPC moved from thread based to lock based synchronization. The thread based synchronization side effect was used by flutter benchmarks in https://github.com/flutter/flutter/blob/master/packages/flutter_tools/lib/src/vmservice.dart#L1223 and https://github.com/flutter/flutter/blob/master/packages/flutter_tools/lib/src/run_hot.dart#L156 to ensure the completeness of the restart/reload and so correct timing. A new RPC _flutter.waitUIThreadIdle is introduced to allow the flutter benchmarks to reintroduce thread based synchronization. Related flutter/flutter#11241

a-siva · 2017-07-18T22:29:29Z

shell/common/platform_view_service_protocol.h

+                              const char** param_values,
+                              intptr_t num_params,
+                              void* user_data,
+                              const char** json_object);


Indentation seems off

a-siva · 2017-07-18T22:34:01Z

shell/common/platform_view_service_protocol.cc

@@ -314,4 +316,29 @@ void PlatformViewServiceProtocol::ScreenshotGpuTask(SkBitmap* bitmap) {
  canvas->flush();
 }

+const char* PlatformViewServiceProtocol::kWaitUIThreadIdleExtensionName =
+    "_flutter.waitUIThreadIdle";


Strictly speaking this is not waiting for the UI thread to idle it is more wait for UI thread to finish processing the first event, I don't have a better name for it though.

How about FlushUIThreadTasks?

It is not actually flushing all the tasks, but just the first one.

Are you sure? The implementation calls blink::Threads::UI()->PostTask, which will queue up a task. Given that the message queue is FIFO, all tasks that were scheduled prior to this task will execute first. No?

I misunderstood the meaning of Tasks, you are right. It will wait for all the previous tasks to be flushed.

yjbanov · 2017-07-18T23:02:44Z

Added @jason-simmons to reviewer list for C++/engine readability.

yjbanov · 2017-07-18T23:06:42Z

shell/common/platform_view_service_protocol.cc

+
+// This API should not be invoked by production code.
+// It can potentially starve the service isolate if the main isolate pauses
+// at a breakpoint or is in an infinite loop.


Might be worth mentioning that this method must be invoked from the VM service thread and blocks it until UI thread tasks are processed.

Should we add this description even to the other services? some of them are blocking too.

That would be great. We have comments like that in shell.h.

yjbanov · 2017-07-18T23:07:06Z

shell/common/platform_view_service_protocol.cc

@@ -314,4 +316,29 @@ void PlatformViewServiceProtocol::ScreenshotGpuTask(SkBitmap* bitmap) {
  canvas->flush();
 }

+const char* PlatformViewServiceProtocol::kWaitUIThreadIdleExtensionName =
+    "_flutter.waitUIThreadIdle";


How about FlushUIThreadTasks?

yjbanov · 2017-07-18T23:08:22Z

shell/common/platform_view_service_protocol.h

@@ -43,6 +43,17 @@ class PlatformViewServiceProtocol {
                         void* user_data,
                         const char** json_object);
  static void ScreenshotGpuTask(SkBitmap* bitmap);
+
+  // This API should not be invoked by production code.


ditto re calling from VM isolate thread

Hixie · 2017-07-19T16:58:00Z

So if I understand correctly, the claim here is that hot_mode_dev_cycle__benchmark hotRestartMillisecondsToFrame used to count the entire cycle of "wait for UI thread to be stable, tell app to restart, then wait for app to be done", and now it only counts "tell app to restart, then wait for app to be done"?

Similarly, is the claim that hot_mode_dev_cycle__benchmark hotReloadMillisecondsToFrame used to not count the time waiting for the app to be stable before triggering the hot reload, but now that it does include that time, the reload takes two SECONDS more?

I find it surprising that both of these benchmarks imply that the app is locked for two seconds every time, reliably. There's very little noise in how much difference the benchmarks are seeing.

B3rn475 · 2017-07-19T18:06:17Z

@Hixie
The claim is that hot_mode_dev_cycle__benchmark hotRestartMillisecondsToFrame used to count:

tell app to restart
wait for the Isolate to start
wait for the app to be done / wait for UI thread to be empty (side effect of synchronously listing the views)

while hot_mode_dev_cycle__benchmark hotReloadMillisecondsToFrame used to count:

hot reload
reassemble
schedule frame

Now instead hot_mode_dev_cycle__benchmark hotRestartMillisecondsToFrame counts:

tell app to restart
wait for the Isolate to start
(no "wait for UI thread to be empty" side effect of asynchronously listing the views)

while hot_mode_dev_cycle__benchmark hotReloadMillisecondsToFrame counts:

wait for the app to be done / wait for UI thread to be empty (side effect of triggering hot reload)
hot reload
reassemble
schedule frame

We are actually counting a part of hotRestartMillisecondsToFrame inside hotReloadMillisecondsToFrame instead.

Hixie · 2017-07-19T19:38:21Z

That implies that it takes ~2500 milliseconds for an app to start ("wait for the app to be done / wait for UI thread to be empty"), and ~2000ms for an idle app to be ready to be hot reloaded, both of which seems exceedingly high.

B3rn475 · 2017-07-19T22:38:08Z

On my device we can see this:

[        ] Benchmarking hot restart
[        ] Performing full restart...
[   +1 ms] Refreshing active FlutterViews before restarting.
[ +262 ms] Syncing files to device A0001...
[        ] DevFS: Starting sync from LocalDirectory: '.../flutter/examples/flutter_gallery'
[        ] Scanning project files
[   +3 ms] Scanning package files
[  +24 ms] Scanning asset files
[   +1 ms] Scanning for deleted files
[   +2 ms] DevFS: Sync finished
[        ] Synced 0.0MB.
[+1543 ms] Isolate is runnable.
[+2781 ms] Restart performed in 4,561ms.
[   +2 ms] Restarted app in 4,623ms.

It takes:

~250 ms to synchronize DevFS (even if nothing is changed)
~1300 ms to get to the IsolateRunnable event
~1200 ms for the UI thread to Flush all the tasks (AFAIK render the application)

Changes introduced in flutter/engine@8ba522e has removed from the `_flutter.listViews` the thread synchronization side effect used during benchmarks. The thread synchronization is restored via the new `_flutter.flushUIThreadTasks` RPC. Fixes #11241 Related flutter/engine#3898

B3rn475 requested review from yjbanov and a-siva July 18, 2017 22:00

googlebot added the cla: yes label Jul 18, 2017

B3rn475 mentioned this pull request Jul 18, 2017

Fix restart/reload benchmark synchronization flutter/flutter#11282

Merged

a-siva approved these changes Jul 18, 2017

View reviewed changes

Fixed indentation

20a552f

yjbanov requested a review from jason-simmons July 18, 2017 23:02

yjbanov reviewed Jul 18, 2017

View reviewed changes

Rename RPC name and Add comments

dd967d0

B3rn475 changed the title ~~Add waitUIThreadIdle service RPC~~ Add flushUIThreadTasks service RPC Jul 19, 2017

B3rn475 merged commit 53c9a70 into flutter:master Jul 19, 2017

B3rn475 deleted the benchmark branch July 19, 2017 22:48

Add flushUIThreadTasks service RPC #3898

Add flushUIThreadTasks service RPC #3898

Uh oh!

Conversation

B3rn475 commented Jul 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

yjbanov commented Jul 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

B3rn475 Jul 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Hixie commented Jul 19, 2017

Uh oh!

B3rn475 commented Jul 19, 2017

Uh oh!

Hixie commented Jul 19, 2017

Uh oh!

B3rn475 commented Jul 19, 2017

Uh oh!

Uh oh!

B3rn475 commented Jul 18, 2017 •

edited

Loading

B3rn475 Jul 18, 2017 •

edited

Loading