Add onReportTimings and FrameRasterizedCallback API #8983

liyuqian · 2019-05-16T05:46:47Z

Using it, a Flutter app can monitor missing frames in the release mode, and a custom Flutter runner (e.g., Fuchsia) can add a custom FrameRasterizedCallback.

Related issues:
flutter/flutter#26154
flutter/flutter#31444
flutter/flutter#32447

Need review as soon as possible so we can merge this before the end of May to catch the milestone.

Tests added:

NoNeedToReportTimingsByDefault
NeedsReportTimingsIsSetWithCallback
ReportTimingsIsCalled
FrameRasterizedCallbackIsCalled
FrameTimingSetsAndGetsProperly
onReportTimings preserves callback zone
FrameTiming.toString has the correct format

This will need a manual engine roll as the TestWindow defined in the framework needs to implement onReportTimings.

liyuqian · 2019-05-16T17:54:19Z

CC @kenzieschmoll @devoncarew : please let me know if this helps DevTools or other IDE tools to plot the performance information (e.g., GPU thread frame raster time).

liyuqian · 2019-05-17T20:02:32Z

I'll also try to see if we can add the C++ callback API defined in #8970 and https://fuchsia-review.googlesource.com/c/topaz/+/281255/1/runtime/flutter_runner/frame_stats.cc#42

kenzieschmoll · 2019-05-17T20:52:15Z

Two Questions:

Can we guarantee that all Timeline trace events will have timestamps within the timestamps of the FrameTiming object?

In the current state of determining frame timing, we use the PipelineItem flow events to signal frame start and end, but the corresponding trace events for the frame can have timestamps outside of the flow event timestamps. For example, sometimes the VSYNC event timestamp will be before the PipelineItem start flow event.

How could we access this FrameTiming object from DevTools?

liyuqian · 2019-05-21T00:35:43Z

@kenzieschmoll : I just scheduled a meeting on Wednesday for us to discuss those questions.

@nathanrogersgoogle : please check if the API added to settings.h here looks good to you

Meanwhile, I'm still figuring out how to add uni test to best exercise this API in the engine repo.

liyuqian · 2019-05-21T22:02:32Z

@chinmaygarde @Hixie @cbracken @nathanrogersgoogle @kenzieschmoll : since flutter/flutter#26154 has a May milestone, it would be really nice if you can start reviewing this PR soon so it can be merged before the end of May.

chinmaygarde · 2019-05-21T21:58:42Z

shell/common/shell_test.cc

@@ -58,6 +59,29 @@ void ShellTest::SetSnapshotsAndAssets(Settings& settings) {
  }
 }

+void ShellTest::PlatformViewNotifyCreated(Shell* shell) {


Good call on moving these to the fixture.

shell/common/shell_test.h

shell/common/animator.h

common/settings.h

Hixie · 2019-05-21T22:21:33Z

lib/stub_ui/lib/src/ui/window.dart

@@ -35,6 +35,71 @@ typedef PlatformMessageResponseCallback = void Function(ByteData data);
 typedef PlatformMessageCallback = void Function(
    String name, ByteData data, PlatformMessageResponseCallback callback);

+/// Time-related performance metrics of a frame.


this needs much more documentation. For example, how to get one of these, whether it makes sense to examine the data at all in debug mode, what it measures, etc.

Added a little more documentation. I'll continue to add more once I come up with other topics while polishing this PR.

lib/stub_ui/lib/src/ui/window.dart

lib/ui/window.dart

Hixie · 2019-05-21T22:34:22Z

Can we disable all this logic in the case where the callback isn't set?

lib/stub_ui/lib/src/ui/window.dart

nathanrogersgoogle · 2019-05-22T13:00:05Z

shell/common/rasterizer.cc

+
+  // TODO(liyuqian): in Fuchsia, the rasterization doesn't finish when
+  // Rasterizer::DoDraw finishes. Future work is needed to adapt the timestamp
+  // for Fuchsia.


What extra work after |Rasterizer::DoDraw| are you referring to specifically? Are you talking about the GPU work (as opposed to the CPU "GPU thread" work in Flutter)? What does the future work you mention here look like? In some Fuchsia traces I had lying around, the GPU thread's work (covered by the "GPURasterizer::Draw") does nearly end with |Rasterizer::DoDraw| (there's just a couple of microseconds after it). If I'm understanding this correctly, I'd be happy to clarify that this is just Flutter GPU thread work here, and have the burden of measuring GPU work be on Flutter runner.

You're right that Rasterizer::DoDraw and Rasterizer::Draw (which adds GPURasterizer::Draw tracing) are approximately equal. The extra work I referred was SceneUpdateContext::ExecutePaintTasks in scene_update_context.cc. I've added this info to the code comment.

liyuqian

Thank you all @Hixie @chinmaygarde @nathanrogersgoogle for your comments! I've updated the PR accordingly. Please check if I missed something.

lib/stub_ui/lib/src/ui/window.dart

lib/ui/window.dart

liyuqian · 2019-05-22T21:04:11Z

lib/stub_ui/lib/src/ui/window.dart

@@ -35,6 +35,71 @@ typedef PlatformMessageResponseCallback = void Function(ByteData data);
 typedef PlatformMessageCallback = void Function(
    String name, ByteData data, PlatformMessageResponseCallback callback);

+/// Time-related performance metrics of a frame.


Added a little more documentation. I'll continue to add more once I come up with other topics while polishing this PR.

common/settings.h

shell/common/shell_test.h

shell/common/animator.h

liyuqian · 2019-05-22T22:45:54Z

shell/common/rasterizer.cc

+
+  // TODO(liyuqian): in Fuchsia, the rasterization doesn't finish when
+  // Rasterizer::DoDraw finishes. Future work is needed to adapt the timestamp
+  // for Fuchsia.


You're right that Rasterizer::DoDraw and Rasterizer::Draw (which adds GPURasterizer::Draw tracing) are approximately equal. The extra work I referred was SceneUpdateContext::ExecutePaintTasks in scene_update_context.cc. I've added this info to the code comment.

chinmaygarde · 2019-05-21T21:56:34Z

shell/common/shell_test.h

@@ -26,11 +26,17 @@ class ShellTest : public ThreadTest {
  ~ShellTest();

  Settings CreateSettingsForFixture();
-
+  std::unique_ptr<Shell> CreateShell(Settings&& settings);


Both the settings and the task runners are copyable and movable. You can get rid of the double &.

chinmaygarde · 2019-06-04T04:52:59Z

shell/common/shell.cc

+
+  // The engine might have been destructed in ~Shell while the rasterizer is
+  // still alive and calling this from OnFrameRasterized.
+  if (!engine_) {


This condition is not possible as the destruction of the shell will only happen after the engine is destroyed.

Removed this racy access to unique_ptr<Engine> engine_ on the GPU thread, and modified GetEngine as we discussed.

Thanks. Updates look safe.

Is this the renewed LGTM? 😄

Oh. Sorry. Yeah :)

Using it, a Flutter app can monitor missing frames in the release mode, and a custom Flutter runner (e.g., Fuchsia) can add a custom FrameRasterizedCallback. Related issues: flutter/flutter#26154 flutter/flutter#31444 flutter/flutter#32447 Need review as soon as possible so we can merge this before the end of May to catch the milestone. Tests added: * NoNeedToReportTimingsByDefault * NeedsReportTimingsIsSetWithCallback * ReportTimingsIsCalled * FrameRasterizedCallbackIsCalled * FrameTimingSetsAndGetsProperly * onReportTimings preserves callback zone * FrameTiming.toString has the correct format This will need a manual engine roll as the TestWindow defined in the framework needs to implement onReportTimings.

…engine#8983)

This will manually land flutter/engine#8983 in the framework.

…engine#8983)

This will catch failure early for cases like #9216 (which fixes manual roll for #8983).

Using it, a Flutter app can monitor missing frames in the release mode, and a custom Flutter runner (e.g., Fuchsia) can add a custom FrameRasterizedCallback. Related issues: flutter/flutter#26154 flutter/flutter#31444 flutter/flutter#32447 Need review as soon as possible so we can merge this before the end of May to catch the milestone. Tests added: * NoNeedToReportTimingsByDefault * NeedsReportTimingsIsSetWithCallback * ReportTimingsIsCalled * FrameRasterizedCallbackIsCalled * FrameTimingSetsAndGetsProperly * onReportTimings preserves callback zone * FrameTiming.toString has the correct format This will need a manual engine roll as the TestWindow defined in the framework needs to implement onReportTimings.

This will catch failure early for cases like flutter#9216 (which fixes manual roll for flutter#8983).

This will manually land flutter/engine#8983 in the framework.

googlebot added the cla: yes label May 16, 2019

liyuqian requested review from chinmaygarde and Hixie May 16, 2019 17:52

liyuqian mentioned this pull request May 20, 2019

Add pipeline_state_observer interface #8970

Closed

liyuqian mentioned this pull request May 21, 2019

Wait for full start for transitions_perf accuracy flutter/flutter#29367

Closed

10 tasks

liyuqian requested review from kenzieschmoll, nathanrogersgoogle and cbracken May 21, 2019 21:58

chinmaygarde reviewed May 21, 2019

View reviewed changes