Reland 5: Multiview pipeline

[dkwingsmt]

This relands #50931.

The crash that caused the 4th revert has been fixed by flutter/flutter#144212.

There has been discussion on why the benchmark in previous attempts show significant drop in build time. This PR addresses it using option a as described in this comment.

This PR also addresses flutter/flutter#144584 with option 1. A test is added.

Also addresses: flutter/flutter#144444

Pre-launch Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene wiki page, which explains my responsibilities.
• I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
• I listed at least one issue that this PR fixes in the description above.
• I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
• I updated/added relevant documentation (doc comments with ///).
• I signed the CLA.
• All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

[dkwingsmt]

cc @loic-sharma @goderbauer @jonahwilliams @chinmaygarde

Let's move the discussion here. (I've not changed anything yet.) I'll let you know when it's ready for review.

[dkwingsmt]

As Michael has pointed out, the metrics of devicelab tests are not recorded using the engine's frame timing recorder, but the timeline API, which corresponds to TimelineTask in the framework and TRACE_EVENT* macros in the engine.

The build time as measured in devicelab tests is defined as the time from the beginning of SchedulerBinding.handleBeginFrame to the end of SchedulerBinding.handleDrawFrame.

https://github.com/flutter/flutter/blob/89b5f3a717b8fd0aaf6536f13ed3d56650f3ef86/packages/flutter/lib/src/scheduler/binding.dart#L1200

https://github.com/flutter/flutter/blob/89b5f3a717b8fd0aaf6536f13ed3d56650f3ef86/packages/flutter/lib/src/scheduler/binding.dart#L1331

I've verified that by somehow including the lines that start up rasterization (see the culprit revision here) into the said range, the resulting metrics return to normal.

The problem now is how to fix it. In multiview Flutter, the engine can't dispatch the layer trees to the pipeline until after handleDrawFrame, and thus simply recording the time in the framework will never include these time.

I came up with two options:

Option a: Change when the layer trees are dispatched

In this option, we keep the framework untouched, and try our best to make the engine dispatch the layer trees before the end of handleDrawFrame. This is done by addressing flutter/flutter#144584.

Basically, flutter/flutter#144584 is an issue that we likely have to address, and by addressing flutter/flutter#144584, the metric problem is solved.

The downside is that, if we go with flutter/flutter#144584 's option 2, which is a much smaller change, then the fix applies to most cases except if the app has multiple views and only render part of them (I call them partially rendered apps ), and in such cases, the metrics will not include the time to start up rasterization again, making the build time look falsely better again.

And if we go with flutter/flutter#144584 's option 1... well, it's another big change stacked up.

Option b: Record the time in the engine

In this option, we remove the two lines where the framework records the build time, and adds TRACE_EVENT* to the engine in proper places. This works for all cases, and is a small change.

The problem is that we're probably implementing flutter/flutter#144584 anyway, so one might argue that the change here is not necessary. Additionally, according to my local testing, this option somehow brings larger variance to the result: before the change the worst build time stays within the range of 8.5-9.5, while after the change it sways between 7.5-10.5.

[goderbauer]

My suggestion is to not mix changing what we measure with introducing the multiview pipeline in the same PR. Those should be separate changes to reduce risk. With that, I would go with option a) since that will also resolve the concerns around flutter/flutter#144445. It is also the option that keeps the current behavior as much as possible which further reduces the likelihood that this change regresses anything.

[dkwingsmt]

I've implemented flutter/flutter#144584 option 2 and #51186 (comment) option a. This PR should not cause benchmark regression any more. I've also added a unit test for it.

This PR is ready for review. For the convenience of reviewing, I created a clean patch that contains the changes compared with the last attempt:
37917f1

[jonahwilliams]

LGTM

[loic-sharma]

This waited until the end of the onDrawFrame callback, right? The end of the vsync is a separate event that happens later.

Consider something like:

Suggested change

	// pipeline a bit earlier than having to wait for the end of the vsync.
	// pipeline a bit earlier than having to wait for the end of the `onDrawFrame` callback.

[dkwingsmt]

I think they're more or less the same event. But anyway, the vsync is not a concept of the RuntimeController. I'm changing the doc to this, what do you think?

  // Tracks the views that have been called `Render` during a frame.
  //
  // If all views that have been registered by `AddView` have been called
  // `Render`, then the runtime controller notifies the client of the end of
  // frame immediately, allowing the client to submit the views to the pipeline
  // a bit earlier than having to wait for the end of `BeginFrame`. See also
  // `Animator::OnAllViewsRendered`.
  //

[loic-sharma]

This is a good comment that is useful context for RuntimeController::Render. I would consider moving (or copying) part of this comment to that function.

[dkwingsmt]

I've thought about it but it's hard because this method is actually implementing its parent class PlatformConfigurationClient::Render. There are a lot of implementation details that I don't know how to explain in the context of PlatformConfigurationClient, or whether we should mention them at all. If you think about it, we basically define most of the methods of PlatformConfigurationClient as simple wrappers for those in PlatformConfiguration, and then for those in PlatformDispatcher or FlutterView. It's weird if we have to document that much for them. What do you think?

[loic-sharma]

Is this used?

[dkwingsmt]

No. Thanks for pointing out.

[auto-submit]

auto label is removed for flutter/engine/51186, due to - The status or check suite Mac mac_clang_tidy has failed. Please fix the issues identified (or deflake) before re-applying this label.

[auto-submit]

auto label is removed for flutter/engine/51186, due to - The status or check suite Mac mac_clang_tidy has failed. Please fix the issues identified (or deflake) before re-applying this label.