Fix Opacity performance regression on Fuchsia #15573

arbreng · 2020-01-14T02:24:34Z

Details and traces at fxbug/43364

mikejurka

LGTM, tested and verified no visual regressions and a 40% reduced frame rasterizer time on benchmarks

mklim

Do you mind describing at a high level what your intended changes to the layer tree and passes are and why you're making them in the PR description? I was asked to give this a review pass but I'm having difficulty figuring out the big picture of all the changes this patch is trying to make and why. Some text going over the broad strokes would help. :) It would also be useful to any future historians who are trying to reason through why a particular change was made while they're debugging.

This patch also needs tests before we can merge it into the tree, see Tree Hygiene #4.

mklim · 2020-01-16T01:47:48Z

flow/layers/child_scene_layer.cc

@@ -31,6 +31,7 @@ void ChildSceneLayer::Preroll(PrerollContext* context, const SkMatrix& matrix) {
 void ChildSceneLayer::Paint(PaintContext& context) const {
  TRACE_EVENT0("flutter", "ChildSceneLayer::Paint");
  FML_DCHECK(needs_painting());
+  FML_DCHECK(needs_system_composite());


Trying to make sure I understand this: So before FuchsiaSystemCompositedLayer would wrap this call. Now we're not constructing it any more, so we're calling it before every paint?

Sure thing,

We landed this PR: #14024 in order to have OpacityLayer work on Fuchsia even with cross-process content. This way you could do say Opacity(opacity: 0.1, children: [chromium_child_view]) and the chromium that you embedded would be rendered at 0.1 group opacity.

In order to do so, OpacityLayer's need to be "popped out" as their own system composited layer. We also wanted to remove this same "popping out" from the PhysicalShapeLayer as it is wasteful in terms of resources. needs_system_composite flag indicates that one of the layers children or the layer itself is going to be popped out.

Unfortunately, the PhysicalShapeLayer part of that change caused a large performance regression on Fuchsia. So, this patch reverts the original one and then re-applies it but without this commit: c4df6df

Definitely agreed on the tests, I'm thinking about that right now. Lack of tests is why the perf regression made it through in the first place. :)

Also, to your previous question: ChildSceneLayer is a layer to embed cross-process content, so by definition needs_system_composite is true for it. I added that DCHECK as a sanity check while debugging

mklim · 2020-01-16T01:52:15Z

flow/layers/layer_tree.cc

@@ -83,7 +83,7 @@ void LayerTree::UpdateScene(SceneUpdateContext& context,
      context,
      SkRRect::MakeRect(
          SkRect::MakeWH(frame_size_.width(), frame_size_.height())),
-      SK_ColorTRANSPARENT);
+      SK_ColorTRANSPARENT, SK_AlphaOPAQUE);


It looks like this is covering Flutter's UI with an opaque rect. On other platforms Flutter UIs are expected to blend with other backgrounds transparently. Is it safe to assume that's never the case here?

Yes, that is safe to assume. I think we would like to have the same blending on Fuchsia, but unfortunately Scenic doesn't support that at the moment; you get a black square if you try

mklim · 2020-01-16T01:52:27Z

flow/layers/layer_tree.h

@@ -14,6 +14,7 @@
 #include "flutter/fml/macros.h"
 #include "flutter/fml/time/time_delta.h"
 #include "third_party/skia/include/core/SkPicture.h"
+#include "third_party/skia/include/core/SkSize.h"


nit: Extra import?

mklim · 2020-01-16T01:53:47Z

flow/layers/elevated_container_layer.cc

-  // able to have developers specify the behavior here to alternatives besides
-  // clamping, like normalization on some arbitrary curve.
-  float clamped_elevation = elevation;
-  if (max_elevation > -1 && (parent_elevation + elevation) > max_elevation) {


(For any other reviewers: this was moved into the SceneUpdateContext. That should keep working.)

I suspect moving this here had something to do with the perfoprmance regression. Out next CL will rethink how flutter elevation -> scenic Z works entirely.

mklim · 2020-01-16T18:55:01Z

flow/layers/opacity_layer.cc

  SkMatrix child_matrix = matrix;
  child_matrix.postTranslate(offset_.fX, offset_.fY);
+
+  total_elevation_ = context->total_elevation;


It looks like we're always adding kOpacityElevationWhenUsingSystemCompositor to the total, but presumably there are some conditions where we're not using the system compositor and opacity layers don't add to the elevation. Are we going to be incorrectly adding to our elevation in those cases?

Good catch

We only use the system compositor if the OpacityLayer has an out-of-process child somewhere (a ChildSceneLayer)

mklim · 2020-01-16T19:11:03Z

flow/layers/physical_shape_layer.cc


-  if (elevation() == 0) {
+  context->total_elevation += elevation_;


I do think it's a potential foot gun to leave it on individual layers to remember to update total_elevation_ correctly if they happen to be elevated. Right now it's only two places so it's reasonable, but this is the type of bug that wouldn't be immediately obvious if a third was introduced for some reason and didn't feed back into the global state. I don't have any immediate suggestions on how to fix it though.

Agree with Michael on this. It might make sense for us unify the layer state management pieces. Maybe making the MutatorStack struct more general would be one way of addressing this.

Yeah, I agree.

Fortunately I think context->total_elevation is going to go away in my next PR and this problem will be moot

iskakaushik

Given that:

This is a partial revert of c4df6df
Test infra for in-tree Fuchsia testing is not at a place where we can identify the regression. Me and @gw280 are working on improving it this quarter.
This addresses a high priority issue and http://fxb/43364 has multiple people who have seem the regression fixed with this change.

@Hixie can we get exemption for testing for this PR.

cbracken · 2020-01-16T22:39:58Z

@iskakaushik given the circumstances, go for it when green.

…#15573)

flutter/engine@839b34c...0427416 git log 839b34c..0427416 --first-parent --oneline 2020-01-16 [email protected] Fix Opacity performance regression on Fuchsia (flutter/engine#15573) 2020-01-16 [email protected] [web] edge launcher for windows (flutter/engine#15690) 2020-01-16 [email protected] [web] Add BoxHeightStyle.strut (flutter/engine#15694) 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] on the revert to ensure that a human is aware of the problem. To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

* Add more profile markers * Revert FuchsiaSystemCompistedLayer changes * Re-add opacity w/o elevation changes * Fix formatting

This reverts commit 79d2b7d.

* Add more profile markers * Revert FuchsiaSystemCompistedLayer changes * Re-add opacity w/o elevation changes * Fix formatting

arbreng requested review from mikejurka, iskakaushik and jason-simmons January 14, 2020 02:24

auto-assign bot requested a review from gw280 January 14, 2020 02:24

googlebot added the cla: yes label Jan 14, 2020

arbreng force-pushed the fix-opacity branch from 4e08563 to 132507d Compare January 15, 2020 02:23

mikejurka approved these changes Jan 15, 2020

View reviewed changes

cbracken requested a review from flar January 16, 2020 00:43

mikejurka requested review from liyuqian, chinmaygarde and cbracken January 16, 2020 04:54

mklim reviewed Jan 16, 2020

View reviewed changes

arbreng added 4 commits January 16, 2020 11:58

Add more profile markers

935f267

Revert FuchsiaSystemCompistedLayer changes

f7e4c10

Re-add opacity w/o elevation changes

b740240

Fix formatting

a6040f2

arbreng force-pushed the fix-opacity branch from 132507d to a6040f2 Compare January 16, 2020 19:58

iskakaushik reviewed Jan 16, 2020

View reviewed changes

iskakaushik merged commit 0427416 into master Jan 16, 2020

arbreng deleted the fix-opacity branch January 17, 2020 00:37

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

0427416 Fix Opacity performance regression on Fuchsia (flutter/engine…

c284c3e

…#15573)

engine-flutter-autoroll mentioned this pull request Jan 17, 2020

Roll engine 839b34cc687c..04274160e60c (3 commits) flutter/flutter#49011

Merged

arbreng added a commit that referenced this pull request Jan 21, 2020

Fix Opacity performance regression on Fuchsia (#15573)

9a1f93f

* Add more profile markers * Revert FuchsiaSystemCompistedLayer changes * Re-add opacity w/o elevation changes * Fix formatting

gnoliyil pushed a commit to gnoliyil/engine that referenced this pull request Jan 31, 2020

Fix Opacity performance regression on Fuchsia (flutter#15573)

647a814

* Add more profile markers * Revert FuchsiaSystemCompistedLayer changes * Re-add opacity w/o elevation changes * Fix formatting

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

Fix Opacity performance regression on Fuchsia (flutter#15573)

79d2b7d

* Add more profile markers * Revert FuchsiaSystemCompistedLayer changes * Re-add opacity w/o elevation changes * Fix formatting

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

Revert "Fix Opacity performance regression on Fuchsia (flutter#15573)"

ce36253

This reverts commit 79d2b7d.

filmil pushed a commit to filmil/engine that referenced this pull request Mar 13, 2020

Fix Opacity performance regression on Fuchsia (flutter#15573)

da4f1d4

* Add more profile markers * Revert FuchsiaSystemCompistedLayer changes * Re-add opacity w/o elevation changes * Fix formatting


		if (elevation() == 0) {
		context->total_elevation += elevation_;

Fix Opacity performance regression on Fuchsia #15573

Fix Opacity performance regression on Fuchsia #15573

Uh oh!

Conversation

arbreng commented Jan 14, 2020

Uh oh!

mikejurka left a comment

Choose a reason for hiding this comment

Uh oh!

mklim left a comment

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!

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!

iskakaushik left a comment

Choose a reason for hiding this comment

Uh oh!

cbracken commented Jan 16, 2020

Uh oh!

Uh oh!