Retained rendering in Fuchsia PhysicalShapeLayer by liyuqian · Pull Request #6558 · flutter/engine

liyuqian · 2018-10-16T18:55:51Z

When this lands/rolls into Fuchsia, a manual roll with https://fuchsia-review.googlesource.com/c/topaz/+/241557 is needed.

chinmaygarde · 2018-10-18T19:49:29Z

flow/layers/physical_shape_layer.cc

  FML_DCHECK(needs_system_composite());

+  if (entity_node_ != nullptr) {
+    if (entity_node_->session() && context.top_entity()) {


Nit: Just add an assertion for this condition and add the child node as normal.

chinmaygarde · 2018-10-24T21:05:15Z

Can you land this please?

liyuqian · 2018-10-24T21:17:13Z

Oops, I was waiting for the framework change to land first. Mark this as wip.

liyuqian · 2018-11-13T16:48:21Z

@chinmaygarde @mikejurka : Please have a look at this PR again as I now changed EntityNode to be stored as unique_ptr according to https://fuchsia-review.googlesource.com/c/garnet/+/224197

See also FL-53 about the performance difference with or without this retained rendering. Here's a short summary:

Before retained rendering,
the RenderFrame in Scenic Compositor takes 22.9ms on average, the GPURasterizer::Draw in flutter takes 23.5ms on average.

After retained rendering,
the RenderFrame in Scenic Compositor takes 15.7ms on average, the GPURasterizer::Draw in flutter takes 7.9ms on average.

mikejurka · 2018-11-13T18:14:34Z

flow/layers/physical_shape_layer.h

+
+#if defined(OS_FUCHSIA)
+  std::unique_ptr<scenic::EntityNode> entity_node_ptr_;
+  mutable bool fuchsia_painted_ = false;


I'm not sure I understand this variable, could you add a comment?

mikejurka · 2018-11-13T18:15:32Z

flow/layers/physical_shape_layer.cc

  FML_DCHECK(needs_painting());

+#if defined(OS_FUCHSIA)
+  if (fuchsia_painted_) {


what is the difference between fuchsia_painted_ and needs_painting() ? When/why do their values diverge?

needs_painting check whether the paint_bounds is empty and it applies to all Flutter platforms.

fuchsia_painted is for Fuchsia only because Fuchsia always allocates a buffer for a PhysicalShapeLayer. Even if the paint_bounds is not empty, we can still use this to optimize performance by only paint the same PhysicalShapeLayer once within Fuchsia.

flow/scene_update_context.h

mikejurka · 2018-11-14T22:33:51Z

flow/layers/physical_shape_layer.h

  std::unique_ptr<scenic::EntityNode> entity_node_ptr_;
+
+  // Whether the Fuchsia scenic::EntityNode of this exact same layer has already
+  // been painted in previous frames. If so, we can skip the Paint phase and


has been painted in a previous frame. This means that the content below the EntityNode has already been rasterized.

"and let Scenic re-use the same Entity nodes."

liyuqian · 2019-01-17T00:15:59Z

@chinmaygarde @mikejurka : please check the updated PR that (I think) correctly manages the scenic resources: (1) only SubmitSurface when the surface is no longer used in retained rendering; (2) let the SurfaceProducer own surfaces and entity nodes so they'll get destructed in the right thread.

mikejurka · 2019-01-23T08:09:19Z

flow/layers/layer_tree.cc

    frame.AddPaintedLayer(root_layer_.get());
  }
  container.AddChild(transform.entity_node());
+  frame.Finalize();


Can this happen in the frame's destructor?

I actually found a way to remove Finalize() and do everything in the destructor :)

mikejurka

Could we keep using unique_ptrs, but have them be moved onto a "used" list somehow?

Alternatively, could we use shared_ptrs and have the SurfaceProducer use custom deleters to take further action when something is no longer referenced?

mikejurka · 2019-01-23T08:13:06Z

flow/layers/physical_shape_layer.cc


  UpdateSceneChildren(context);
+
+  context.SetRetainedSurface(key, frame.Finalize());


Nevermind, I see why you had to create a Finalize()

Nevermind, I found a way to remove Finalize()

mikejurka · 2019-01-23T08:13:50Z

flow/scene_update_context.cc

-SceneUpdateContext::Frame::~Frame() {
-  context().CreateFrame(entity_node(), rrect_, color_, paint_bounds_,
-                        std::move(paint_layers_));
+SceneUpdateContext::SurfaceProducerSurface*


Can we keep the destructor, and have it call Finalize if Finalize() was not already called?
Also, can we add a boolean to make sure we're finalized only once?

I just removed Finalize() and restored the destructor.

mikejurka · 2019-01-23T08:15:09Z

flow/layers/physical_shape_layer.cc

+  // won't call PhysicalShapeLayer::Paint.
+  LayerRasterCacheKey key(
+      this, SkMatrix::MakeScale(context.ScaleX(), context.ScaleY()));
+  SceneUpdateContext::SurfaceProducerSurface* retained_surface =


Could we keep unique_ptrs to surfaces, and then just place them in a Used struct somewhere?

Sure, I just removed all raw pointers and restore the unique_ptr everywhere.

mikejurka · 2019-01-23T08:16:52Z

flow/layers/physical_shape_layer.cc

+    return;
+  }
+
  SceneUpdateContext::Frame frame(context, frameRRect_, color_, elevation_);


Add a comment: "If we can't find an existing retained surface, create one.

mikejurka · 2019-01-23T08:24:00Z

flow/layers/physical_shape_layer.cc

  FML_DCHECK(needs_system_composite());

+  // Retained rendering: speedup by reusing a retained entity node if possible.
+  // When an entity node is reused, no paint layer is added to the frame so we


"When a entity node is resued, no paint layer is added to the frame"
I guess I'm still confused as to how the Layers work here, but: Does this mean that the contents of it haven't changed?

Right, all engine layers (flow::Layer descendents) are currently immutable once constructed from the SceneBuilder. Unfortunately, the fields of those layers don't seem to be immutable due to set field methods. But we only call those set field methods in SceneBuilder. I'll change those fields to "const" in the future and update the SceneBuilder accordingly to reduce the confusion.

mikejurka · 2019-01-23T08:27:17Z

flow/layers/layer_tree.cc

    frame.AddPaintedLayer(root_layer_.get());
  }
  container.AddChild(transform.entity_node());
+  frame.Finalize();


liyuqian

Thanks @mikejurka for your helpful comments! Now I found a way to preserve all the std::unique_ptrs and ~Frame. I hope that this makes the change safer and easier to understand.

liyuqian · 2019-01-23T23:18:58Z

flow/layers/physical_shape_layer.cc

  FML_DCHECK(needs_system_composite());

+  // Retained rendering: speedup by reusing a retained entity node if possible.
+  // When an entity node is reused, no paint layer is added to the frame so we


Right, all engine layers (flow::Layer descendents) are currently immutable once constructed from the SceneBuilder. Unfortunately, the fields of those layers don't seem to be immutable due to set field methods. But we only call those set field methods in SceneBuilder. I'll change those fields to "const" in the future and update the SceneBuilder accordingly to reduce the confusion.

liyuqian · 2019-01-25T18:39:43Z

flow/layers/layer_tree.cc

    frame.AddPaintedLayer(root_layer_.get());
  }
  container.AddChild(transform.entity_node());
+  frame.Finalize();


I actually found a way to remove Finalize() and do everything in the destructor :)

liyuqian · 2019-01-25T18:40:38Z

flow/layers/physical_shape_layer.cc

+  // won't call PhysicalShapeLayer::Paint.
+  LayerRasterCacheKey key(
+      this, SkMatrix::MakeScale(context.ScaleX(), context.ScaleY()));
+  SceneUpdateContext::SurfaceProducerSurface* retained_surface =


Sure, I just removed all raw pointers and restore the unique_ptr everywhere.

liyuqian · 2019-01-25T18:41:05Z

flow/layers/physical_shape_layer.cc

+    return;
+  }
+
  SceneUpdateContext::Frame frame(context, frameRRect_, color_, elevation_);


liyuqian · 2019-01-25T18:41:21Z

flow/layers/physical_shape_layer.cc


  UpdateSceneChildren(context);
+
+  context.SetRetainedSurface(key, frame.Finalize());


Nevermind, I found a way to remove Finalize()

liyuqian · 2019-01-25T18:42:12Z

flow/scene_update_context.cc

-SceneUpdateContext::Frame::~Frame() {
-  context().CreateFrame(entity_node(), rrect_, color_, paint_bounds_,
-                        std::move(paint_layers_));
+SceneUpdateContext::SurfaceProducerSurface*


I just removed Finalize() and restored the destructor.

mikejurka · 2019-02-01T18:45:32Z

LGTM

…ine#6558)

flutter/engine@85492c3...4812a2a git log 85492c3..4812a2a --no-merges --oneline 4812a2a DCHECK that clip layer's behavior isn't none (flutter/engine#7659) eaae8a6 Retained rendering in Fuchsia PhysicalShapeLayer (flutter/engine#6558) 1946082 Move Brightness definition to dart:ui (#27479) (flutter/engine#7678) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff ([email protected]), and stop the roller if necessary.

flutter/engine@85492c3...4812a2a git log 85492c3..4812a2a --no-merges --oneline 4812a2a DCHECK that clip layer&flutter#39;s behavior isn&flutter#39;t none (flutter/engine#7659) eaae8a6 Retained rendering in Fuchsia PhysicalShapeLayer (flutter/engine#6558) 1946082 Move Brightness definition to dart:ui (flutter#27479) (flutter/engine#7678) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff ([email protected]), and stop the roller if necessary.

googlebot added the cla: yes label Oct 16, 2018

liyuqian changed the title ~~[wip] Retained rendering in Fuchsia PhysicalShapeLayer~~ Retained rendering in Fuchsia PhysicalShapeLayer Oct 16, 2018

liyuqian requested a review from chinmaygarde October 16, 2018 19:16

chinmaygarde approved these changes Oct 18, 2018

View reviewed changes

liyuqian changed the title ~~Retained rendering in Fuchsia PhysicalShapeLayer~~ [wip] Retained rendering in Fuchsia PhysicalShapeLayer Oct 24, 2018

liyuqian mentioned this pull request Oct 25, 2018

Explore more retained rendering flutter/flutter#23535

Closed

2 tasks

liyuqian force-pushed the fuchsia branch from 637c916 to 8c63acc Compare November 8, 2018 20:34

liyuqian changed the title ~~[wip] Retained rendering in Fuchsia PhysicalShapeLayer~~ Retained rendering in Fuchsia PhysicalShapeLayer Nov 13, 2018

liyuqian requested a review from mikejurka November 13, 2018 16:44

chinmaygarde approved these changes Nov 13, 2018

View reviewed changes

mikejurka reviewed Nov 13, 2018

View reviewed changes

mikejurka reviewed Nov 14, 2018

View reviewed changes

liyuqian force-pushed the fuchsia branch from 6713908 to aa315a4 Compare November 14, 2018 22:58

liyuqian force-pushed the fuchsia branch from aa315a4 to b128272 Compare December 5, 2018 22:00

liyuqian force-pushed the fuchsia branch 2 times, most recently from eaf2663 to dc90dc0 Compare January 15, 2019 20:41

liyuqian changed the title ~~Retained rendering in Fuchsia PhysicalShapeLayer~~ [wip] Retained rendering in Fuchsia PhysicalShapeLayer Jan 15, 2019

liyuqian force-pushed the fuchsia branch from 591a34f to acd6884 Compare January 16, 2019 23:32

liyuqian changed the title ~~[wip] Retained rendering in Fuchsia PhysicalShapeLayer~~ Retained rendering in Fuchsia PhysicalShapeLayer Jan 16, 2019

liyuqian force-pushed the fuchsia branch from a0f8970 to 137ec67 Compare January 17, 2019 00:13

mikejurka reviewed Jan 23, 2019

View reviewed changes

liyuqian force-pushed the fuchsia branch from 137ec67 to f7c6b62 Compare January 25, 2019 19:05

liyuqian commented Jan 25, 2019

View reviewed changes

Retained rendering in Fuchsia PhysicalShapeLayer

b08e4f6

liyuqian force-pushed the fuchsia branch from f7c6b62 to b08e4f6 Compare January 25, 2019 22:40

Make GetRetainedNode non-const as Nathan suggests

9f1b144

Clang-format

e2f8214

liyuqian merged commit eaae8a6 into flutter:master Feb 4, 2019

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 5, 2019

eaae8a6 Retained rendering in Fuchsia PhysicalShapeLayer (flutter/eng…

75e0ed2

…ine#6558)

engine-flutter-autoroll mentioned this pull request Feb 5, 2019

Roll engine 85492c3e804a..4812a2af115c (3 commits) flutter/flutter#27518

Merged


		UpdateSceneChildren(context);

		context.SetRetainedSurface(key, frame.Finalize());

Conversation

liyuqian commented Oct 16, 2018 • 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!

chinmaygarde commented Oct 24, 2018

Uh oh!

liyuqian commented Oct 24, 2018

Uh oh!

liyuqian commented Nov 13, 2018

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liyuqian commented Jan 17, 2019

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!

mikejurka 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!

liyuqian 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!

mikejurka commented Feb 1, 2019

Uh oh!

Reviewers

Assignees

Labels

liyuqian commented Oct 16, 2018 •

edited

Loading