-
Notifications
You must be signed in to change notification settings - Fork 5.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reland 5: Multiview pipeline #51186
Reland 5: Multiview pipeline #51186
Conversation
…pipeline' into revert-50985-revert-reland-4-mv-pipeline
…0985-revert-reland-4-mv-pipeline
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. |
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 The build time as measured in 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 I came up with two options: Option a: Change when the layer trees are dispatchedIn this option, we keep the framework untouched, and try our best to make the engine dispatch the layer trees before the end of 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 engineIn this option, we remove the two lines where the framework records the build time, and adds 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. |
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. |
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: |
…eland-4-mv-pipeline
cbdd250
to
37917f1
Compare
37917f1
to
6d5623e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
runtime/runtime_controller.h
Outdated
// | ||
// If all registered views (through `AddView`) have been rendered, then the | ||
// end of frame will be called immediately, submitting the views to the | ||
// pipeline a bit earlier than having to wait for the end of the vsync. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
// 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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`.
//
runtime/runtime_controller.h
Outdated
// | ||
// If all registered views (through `AddView`) have been rendered, then the | ||
// end of frame will be called immediately, submitting the views to the | ||
// pipeline a bit earlier than having to wait for the end of the vsync. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
@@ -142,6 142,10 @@ external void sayHiFromFixturesAreFunctionalMain(); | |||
@pragma('vm:external-name', 'NotifyNative') | |||
external void notifyNative(); | |||
|
|||
@pragma('vm:entry-point') | |||
@pragma('vm:external-name', 'NotifyNative2') | |||
external void notifyNative2(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. Thanks for pointing out.
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 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. |
…144747) flutter/engine@03ebd64...6c1751b 2024-03-07 [email protected] Roll Dart SDK from 3ad64881a209 to 2cdf8f23c55f (2 revisions) (flutter/engine#51247) 2024-03-07 [email protected] Reland 5: Multiview pipeline (flutter/engine#51186) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/ doc/main/autoroll/README.md
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
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.