Share engine layers with the framework #6406

liyuqian · 2018-10-02T18:14:04Z

To make the PR minimal, we currently only share the engine layer when pushPhysicalShape (for Fuchsia) or pushOffset (for RepaintBoundary and Opacity) are called. They should be sufficient for our short-term perf goal. In the future, we can incrementally share more engine layers with the framework.

flutter/flutter#21756

chinmaygarde

Some nits. But lgtm otherwise.

chinmaygarde · 2018-10-02T18:28:47Z

lib/ui/painting/engine_layer.h

+
+class EngineLayer;
+
+using RetainedLayer = fml::RefPtr<EngineLayer>;


IMO this typedef does not add to the clarity of the code. fml::RefPtr<EngineLayer> reads clearer. We also haven't gotten into the habit of typedeffing such things in other parts of the engine.

chinmaygarde · 2018-10-02T18:30:34Z

lib/ui/painting/engine_layer.h

+using RetainedLayer = fml::RefPtr<EngineLayer>;
+
+class EngineLayer : public RefCountedDartWrappable<EngineLayer> {
+  DEFINE_WRAPPERTYPEINFO();


Nit: These go at the bottom of the class definition. Though I always get confused with the code style in lib/ui as it was written in blink style.

liyuqian

Thanks for the comments!

liyuqian · 2018-10-02T19:50:47Z

lib/ui/painting/engine_layer.h

+
+class EngineLayer;
+
+using RetainedLayer = fml::RefPtr<EngineLayer>;


liyuqian · 2018-10-02T19:50:52Z

lib/ui/painting/engine_layer.h

+using RetainedLayer = fml::RefPtr<EngineLayer>;
+
+class EngineLayer : public RefCountedDartWrappable<EngineLayer> {
+  DEFINE_WRAPPERTYPEINFO();


flutter/engine@f6af1f2...9ccc966 git log f6af1f2..9ccc966 --no-merges --oneline 9ccc966 Share engine layers with the framework (flutter/engine#6406) fa719e3 Roll src/third_party/skia cc4dbfcfbd8a..6719fcc43b1e (17 commits) (flutter/engine#6409) dc2634d Force lf-line endings for so that source offsets match across platforms. (flutter/engine#6408) 44a5149 Roll src/third_party/skia 7dae882574d2..cc4dbfcfbd8a (15 commits) (flutter/engine#6405) de32c65 Roll src/third_party/skia 3b8b11e1f912..7dae882574d2 (15 commits) (flutter/engine#6403) 3a9c22a Allow GetRectsForRange to provide more detailed/nuanced metrics through RectStyle enum. (flutter/engine#6335) b59c864 Rename the Android fragment support library JAR (flutter/engine#6400) 4213ac1 Add an Android fragment support library to third_party (flutter/engine#6384) a785b25 do not count Hidden nodes at the beginning of the scrollable (flutter/engine#6381) 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, who should be CC'd on the roll, and stop the roller if necessary.

This reverts commit 9ccc966.

Reverts #6406 We need to fix the SkiaGPUObject issue of the raster cache SkImage before merging this PR.

flutter/engine@f6af1f2...2de87a8 git log f6af1f2..2de87a8 --no-merges --oneline 2de87a8 Roll src/third_party/skia 6719fcc43b1e..deb512045e6a (7 commits) (flutter/engine#6411) 68a42e3 Add nullability annotations to MethodChannel/MethodCall. (flutter/engine#6393) 71ba20a Dart SDK roll for 2018-10-02 to d2c5a24fd9ead97a7f18d02786e679293cc3709e (flutter/engine#6410) 9ccc966 Share engine layers with the framework (flutter/engine#6406) fa719e3 Roll src/third_party/skia cc4dbfcfbd8a..6719fcc43b1e (17 commits) (flutter/engine#6409) dc2634d Force lf-line endings for so that source offsets match across platforms. (flutter/engine#6408) 44a5149 Roll src/third_party/skia 7dae882574d2..cc4dbfcfbd8a (15 commits) (flutter/engine#6405) de32c65 Roll src/third_party/skia 3b8b11e1f912..7dae882574d2 (15 commits) (flutter/engine#6403) 3a9c22a Allow GetRectsForRange to provide more detailed/nuanced metrics through RectStyle enum. (flutter/engine#6335) b59c864 Rename the Android fragment support library JAR (flutter/engine#6400) 4213ac1 Add an Android fragment support library to third_party (flutter/engine#6384) a785b25 do not count Hidden nodes at the beginning of the scrollable (flutter/engine#6381) 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, who should be CC'd on the roll, and stop the roller if necessary.

flutter/engine@f6af1f2...74662ab git log f6af1f2..74662ab --no-merges --oneline 74662ab Revert "Share engine layers with the framework" (flutter/engine#6412) 2de87a8 Roll src/third_party/skia 6719fcc43b1e..deb512045e6a (7 commits) (flutter/engine#6411) 68a42e3 Add nullability annotations to MethodChannel/MethodCall. (flutter/engine#6393) 71ba20a Dart SDK roll for 2018-10-02 to d2c5a24fd9ead97a7f18d02786e679293cc3709e (flutter/engine#6410) 9ccc966 Share engine layers with the framework (flutter/engine#6406) fa719e3 Roll src/third_party/skia cc4dbfcfbd8a..6719fcc43b1e (17 commits) (flutter/engine#6409) dc2634d Force lf-line endings for so that source offsets match across platforms. (flutter/engine#6408) 44a5149 Roll src/third_party/skia 7dae882574d2..cc4dbfcfbd8a (15 commits) (flutter/engine#6405) de32c65 Roll src/third_party/skia 3b8b11e1f912..7dae882574d2 (15 commits) (flutter/engine#6403) 3a9c22a Allow GetRectsForRange to provide more detailed/nuanced metrics through RectStyle enum. (flutter/engine#6335) b59c864 Rename the Android fragment support library JAR (flutter/engine#6400) 4213ac1 Add an Android fragment support library to third_party (flutter/engine#6384) a785b25 do not count Hidden nodes at the beginning of the scrollable (flutter/engine#6381) 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, who should be CC'd on the roll, and stop the roller if necessary.

Hixie · 2018-10-04T07:49:18Z

lib/ui/compositing.dart

+  ///
+  /// All the engine layers that are in the subtree of the retained layer will
+  /// be automatically appended to the current engine layer tree. Therefore,
+  /// once this is called, there's no need to call [addToScene] for its children


Mentioning addToScene here is a bit of a non-sequitur, since it's not part of this API. I think it's fine to mention it, but we'd have to add a lot more context, e.g. referring to "when implementing a subclass of the [Layer] concept defined in the rendering layer of Flutter's framework...".

Done. See https://github.com/flutter/engine/pull/6468/files#diff-f5a17f9058eed1b025ee67302ec27aeeR199

Hixie · 2018-10-04T07:56:41Z

With this approach, the retained layer API doesn't provide much benefit for nodes far from the leaves (since there's no way to update a retained layer, as far as I can tell). Is that intentional? We could go much further and make the layer tree persistent and mutable, in the same way the RenderObject tree is. Or we could make it persistent and react-style, as in the widget tree. It's not entirely clear to me what is expensive here, so it's hard to precisely suggest the best API to unlock the most optimisation.

liyuqian · 2018-10-08T21:46:04Z

@Hixie Yes, no way to update a retained layer is intentional in the current design: modifying or creating a new layer tree is cheap (this is probably not true for RenderObject tree) while rasterizing a layer tree is costly. The current retained rendering is trying to save the rasterization time with which mutating the layer tree can't help much: if a leaf node and its pixel changes, it will invalidate the raster cache of all its ancestors no matter whether we persist them or not.

But I haven't had too much time to think about what the optimal solution is so there's likely a lot of room to improve over the current design. I hope that the current design is kept minimal and clean so we can easily incrementally improve it in the future. We can also have more discussions offline as this topic might be too big for just one or two Github comments.

liyuqian added 2 commits October 1, 2018 13:25

std::unique_ptr<Layer> to std::shared_ptr<Layer>

eacacb9

Return the engine layer to Dart

d3ecc4f

liyuqian requested review from Hixie and chinmaygarde October 2, 2018 18:14

googlebot added the cla: yes label Oct 2, 2018

liyuqian mentioned this pull request Oct 2, 2018

Maintain dirtiness and use retained engine layers flutter/flutter#22566

Closed

chinmaygarde approved these changes Oct 2, 2018

View reviewed changes

No RetainedLayer typedef and other nits

f86ecc6

liyuqian commented Oct 2, 2018

View reviewed changes

liyuqian added 2 commits October 2, 2018 13:14

Add missing return

d23726b

Licenses

65d61e3

liyuqian merged commit 9ccc966 into flutter:master Oct 2, 2018

engine-flutter-autoroll mentioned this pull request Oct 2, 2018

Roll engine f6af1f20ba92..9ccc96625f0a (9 commits) flutter/flutter#22585

Closed

liyuqian added a commit that referenced this pull request Oct 2, 2018

Revert "Share engine layers with the framework (#6406)"

98edba8

This reverts commit 9ccc966.

liyuqian mentioned this pull request Oct 2, 2018

Revert "Share engine layers with the framework" #6412

Merged

liyuqian added a commit that referenced this pull request Oct 2, 2018

Revert "Share engine layers with the framework" (#6412)

74662ab

Reverts #6406 We need to fix the SkiaGPUObject issue of the raster cache SkImage before merging this PR.

engine-flutter-autoroll mentioned this pull request Oct 3, 2018

Roll engine f6af1f20ba92..2de87a80f550 (12 commits) flutter/flutter#22597

Closed

engine-flutter-autoroll mentioned this pull request Oct 3, 2018

Roll engine f6af1f20ba92..74662ab69523 (13 commits) flutter/flutter#22599

Merged

Hixie reviewed Oct 4, 2018

View reviewed changes

liyuqian deleted the shared_layers_small branch November 12, 2018 18:02


		class EngineLayer;

		using RetainedLayer = fml::RefPtr<EngineLayer>;

Share engine layers with the framework #6406

Share engine layers with the framework #6406

Uh oh!

Conversation

liyuqian commented Oct 2, 2018

Uh oh!

chinmaygarde left a comment

Choose a reason for hiding this comment

Uh oh!

chinmaygarde Oct 2, 2018

Choose a reason for hiding this comment

Uh oh!

liyuqian Oct 2, 2018

Choose a reason for hiding this comment

Uh oh!

chinmaygarde Oct 2, 2018

Choose a reason for hiding this comment

Uh oh!

liyuqian Oct 2, 2018

Choose a reason for hiding this comment

Uh oh!

liyuqian left a comment

Choose a reason for hiding this comment

Uh oh!

liyuqian Oct 2, 2018

Choose a reason for hiding this comment

Uh oh!

liyuqian Oct 2, 2018

Choose a reason for hiding this comment

Uh oh!

Hixie Oct 4, 2018

Choose a reason for hiding this comment

Uh oh!

liyuqian Oct 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hixie commented Oct 4, 2018

Uh oh!

liyuqian commented Oct 8, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

liyuqian Oct 8, 2018 •

edited

Loading