Maintain dirtiness and use retained engine layers by liyuqian · Pull Request #23434 · flutter/flutter

liyuqian · 2018-10-23T23:13:05Z

The old PR is broken by Github again... I'll try to not reference that PR here to see if this PR will stay unbroken.

test Keep LeaderLayer and FollowerLayer dirty Update subtree dirtiness for widget inspector and screenshot Remove test code Update packages/flutter/lib/src/material/page_transitions_theme.dart Update packages/flutter/lib/src/rendering/layer.dart Update packages/flutter/lib/src/rendering/layer.dart Update packages/flutter/lib/src/rendering/object.dart Fixes Make addToScene & updateSubtreeDirtiness protected We make a new API buildScene to encapsulate those calls. Capture offset in OpacityLayer Add unit tests Remove test code Suppress invalid_use_of_protected_member in tests Remove warning Remove obsolete code Make markDirty protected

Hixie · 2018-10-24T18:22:16Z

+  /// be always considered as dirty.
+  /// {@endtemplate}
+  @override
+  bool get isDirty => true;


The danger of overriding isDirty is that it will likely lead to bugs in the superclass code when that code is written assuming that _isDirty and isDirty are equivalent. I think it would be safer if we had an explicit getter that expressed whether or not a layer needs to always be walked each scene, and then have the logic that tests for dirtiness also check for that flag.

This would have the other advantage that it would mean isDirty wasn't exposed at all, which makes it less likely that people will write code that depends on it (always a bit sketchy).

Hixie · 2018-10-24T18:23:11Z

+  /// This would enable the retained rendering of this layer if it stays clean
+  /// for future frames.
+  @protected
+  void markClean() => _isDirty = false;


looks like this is only used in tests, so it can probably be safely removed (the one local call can be replaced by just setting _isDirty directly).

Done with debugMarkClean.

Hixie · 2018-10-24T18:24:24Z

+  ui.EngineLayer _engineLayer;
+
+  /// Traverse the layer tree and compute if any subtree has a dirty layer (by
+  /// calling [markDirty]). The [ContainerLayer] will override this to respect


"will" implies it doesn't yet.

Also, as with other places, please make sure all dartdocs have a single explanatory sentence in a standalone paragraph first, then additional material in subsequent paragraphs. The first paragraph is used for summaries in tables of contents, autocomplete, etc.

Hixie · 2018-10-24T18:24:50Z

+  ui.EngineLayer _engineLayer;
+
+  /// Traverse the layer tree and compute if any subtree has a dirty layer (by
+  /// calling [markDirty]). The [ContainerLayer] will override this to respect


"by calling markDirty" implies that the computation calls markDirty, which seems unlikely to be what you intended

(maybe "if any subtree has a layer that has been marked dirty using [markDirty]" or some such)

Hixie · 2018-10-24T18:26:20Z

+  ///
+  /// (This is valid after calling [updateSubtreeDirtiness].)
+  @protected
+  bool get isSubtreeDirty => _isSubtreeDirty;


As far as I can tell, you don't need to expose this. You only use it where you can get to the private field directly.

Done with debugIsSubtreeDirty.

Hixie · 2018-10-24T18:26:47Z

+  /// calling [markDirty]). The [ContainerLayer] will override this to respect
+  /// its children.
+  @protected
+  void updateSubtreeDirtiness() {


since implementing this requires direct access to a private variable, you might as well make this private too

I need to override it in ContainerLayer and private method can't be overridden...

Hixie · 2018-10-24T18:27:40Z

@@ -168,8 +229,9 @@ class PictureLayer extends Layer {
  bool willChangeHint = false;


should changing these other attributes also flag the layer as dirty?

Since this only affects cache behavior, not the rendering result, it's Ok to not dirty the layer here. But there are probably many other mutable fields that I should mark dirty once they get changed.

After a second thought, I added getters and setters to them just for consistency. (I've added so many getters and setters, it doesn't matter to add two more...)

Hixie · 2018-10-24T18:28:35Z

+  @override
+  void updateSubtreeDirtiness() {
+    _isSubtreeDirty = isDirty;
+    for (Layer child = firstChild; child != null; child = child.nextSibling) {


consider using a while loop, it may be easier to read (your call)

Hixie · 2018-10-24T18:30:28Z

    assert(child.attached == attached);
    assert(_debugUltimatePreviousSiblingOf(child, equals: firstChild));
    assert(_debugUltimateNextSiblingOf(child, equals: lastChild));
+    markDirty();


this should probably be done in Layer.dropChild, not ContainerLayer._removeChild, in case we ever add another kind of layer that has children.

Hixie · 2018-10-24T18:30:46Z

      assert(node != child); // indicates we are about to create a cycle
      return true;
    }());
+    markDirty();


this should probably be done in Layer.adoptChild

Hixie · 2018-10-24T18:32:04Z

  /// pipeline.
-  Offset offset;
+  Offset get offset => _offset;
+  set offset(Offset offset) {


(here and elsewhere) we generally prefer the name value for the argument to the setter, it makes for easier copy/paste later. (see the style guide)

Hixie · 2018-10-24T18:44:57Z

  int alpha;

+  /// Offset from parent in the parent's coordinate system.
+  Offset offset;


should changing this or the alpha mark the layer as dirty?

Yes, adding similar getters and setters to this Layer and other layers with mutable fields. Meanwhile, I'll also make as many Layer fields final as possible to minimize the change in this PR. If a Layer's fields become mutable in the future, we can add the related logic to just that Layer in that future PR. That should be much cleaner than a big change to every Layer in this PR.

Hixie · 2018-10-24T18:46:01Z

This is pretty close. The one main thing I think we should consider before landing is whether all the mutable fields on all the layer objects that exist today should be adjusted to mark the layer as dirty when mutated. Before they didn't because we assumed that we always walked the entire tree.

liyuqian

I've now either marked the layer fields as final, or handled their needsAddToScene (the old isDirty) accordingly in the setter.

liyuqian · 2018-10-24T22:24:39Z

+  /// This would enable the retained rendering of this layer if it stays clean
+  /// for future frames.
+  @protected
+  void markClean() => _isDirty = false;


Done with debugMarkClean.

liyuqian · 2018-10-24T22:24:52Z

+  ///
+  /// (This is valid after calling [updateSubtreeDirtiness].)
+  @protected
+  bool get isSubtreeDirty => _isSubtreeDirty;


Done with debugIsSubtreeDirty.

liyuqian · 2018-10-24T22:30:04Z

+  ui.EngineLayer _engineLayer;
+
+  /// Traverse the layer tree and compute if any subtree has a dirty layer (by
+  /// calling [markDirty]). The [ContainerLayer] will override this to respect


liyuqian · 2018-10-24T22:31:16Z

+  /// calling [markDirty]). The [ContainerLayer] will override this to respect
+  /// its children.
+  @protected
+  void updateSubtreeDirtiness() {


I need to override it in ContainerLayer and private method can't be overridden...

liyuqian · 2018-10-24T22:33:08Z

@@ -168,8 +229,9 @@ class PictureLayer extends Layer {
  bool willChangeHint = false;


Since this only affects cache behavior, not the rendering result, it's Ok to not dirty the layer here. But there are probably many other mutable fields that I should mark dirty once they get changed.

liyuqian · 2018-10-24T22:33:24Z

+  @override
+  void updateSubtreeDirtiness() {
+    _isSubtreeDirty = isDirty;
+    for (Layer child = firstChild; child != null; child = child.nextSibling) {


liyuqian · 2018-10-24T22:52:03Z

    assert(child.attached == attached);
    assert(_debugUltimatePreviousSiblingOf(child, equals: firstChild));
    assert(_debugUltimateNextSiblingOf(child, equals: lastChild));
+    markDirty();


liyuqian · 2018-10-24T22:53:26Z

  /// pipeline.
-  Offset offset;
+  Offset get offset => _offset;
+  set offset(Offset offset) {


liyuqian · 2018-10-24T23:02:52Z

  int alpha;

+  /// Offset from parent in the parent's coordinate system.
+  Offset offset;


Yes, adding similar getters and setters to this Layer and other layers with mutable fields. Meanwhile, I'll also make as many Layer fields final as possible to minimize the change in this PR. If a Layer's fields become mutable in the future, we can add the related logic to just that Layer in that future PR. That should be much cleaner than a big change to every Layer in this PR.

liyuqian · 2018-10-24T23:32:31Z

+  /// be always considered as dirty.
+  /// {@endtemplate}
+  @override
+  bool get isDirty => true;


Hixie · 2018-10-25T17:37:29Z

+
+  /// Mark that this layer has changed and [addToScene] needs to be called.
+  @protected
+  void markNeedsAddToScene() => _needsAddToScene = true;


trivial nit: use a block for this, otherwise you're returning bool despite being declared void

Hixie · 2018-10-25T17:38:09Z

+    }());
+  }
+
+  /// Subclass may override this to true to disable retained rendering.


Either "Subclasses", plural, or "A subclass".

Hixie · 2018-10-25T17:40:09Z

+      markNeedsAddToScene();
+    }
+  }
+  Rect _overlayRect;


nit: (here and elsewhere) style guide suggests the order getter, field, setter.

Hixie · 2018-10-25T17:41:04Z


+  @override
+  void updateSubtreeNeedsAddToScene() {
+    _subtreeNeedsAddToScene = _needsAddToScene || alwaysNeedsAddToScene;


it's a pity ContainerLayer directly accesses private fields of its superclass (it means e.g. that you can't create a different implementation of ContainerLayer), but I don't immediately see a clean way to fix this, so, oh well.

Oh, I expected Dart Analysis to give me errors if a subclass is accessing its parent class' private fields! It seems to be not the case and I have to be careful by myself. I think super.updateSubtreeNeedsAddToScene() would work well in this case.

Hixie · 2018-10-25T17:42:02Z

+    markNeedsAddToScene();
+    super.adoptChild(child);
+  }
+


i'd probably put this in Layer, not ContainerLayer, but I guess it doesn't matter.

Hixie · 2018-10-25T17:42:58Z

cc @goderbauer who may want to review this.

zanderso · 2018-10-26T05:04:51Z

  S find<S>(Offset regionOffset);

  /// Override this method to upload this layer to the engine.
-  void addToScene(ui.SceneBuilder builder, [Offset layerOffset = Offset.zero]);


DBC

This is a breaking change, and will require a manual roll into Fuchsia. See: https://logs.chromium.org/logs/fuchsia/buildbucket/cr-buildbucket.appspot.com/8931658542863723648/+/steps/build/0/steps/build_fuchsia/0/steps/ninja/0/stdout

A patch was actually ready to fix it since 10/11: https://fuchsia-review.googlesource.com/c/topaz/+/211549

However, I forgot to add tag back then and now the presubmit somehow always gets red. Do you or @rmacnak-google know what's wrong with topaz-x64-perf-dawson_canyon there?

Update: https://fuchsia-review.googlesource.com/c/topaz/+/211549 has landed so it must be a flaky test. Can we kick an auto roll into Fuchsia to see if succeeds this time?

Did we make a a breaking change announcement for this?
This breaks anyone that extends Layer, e.g it broke #23412

liyuqian · 2018-10-31T15:15:57Z

No, we missed it. Will post an announcement today. Thanks for the remind!

…

On Wed, Oct 31, 2018, 7:55 AM Amir Hardon ***@***.*** wrote: ***@***.**** commented on this pull request. ------------------------------ In packages/flutter/lib/src/rendering/layer.dart <#23434 (comment)>: > @@ -104,7 +169,29 @@ abstract class Layer extends AbstractNode with DiagnosticableTreeMixin { S find<S>(Offset regionOffset); /// Override this method to upload this layer to the engine. - void addToScene(ui.SceneBuilder builder, [Offset layerOffset = Offset.zero]); Did we make a a breaking change announcement for this? This breaks anyone that extends Layer, e.g it broke #23412 <#23412> — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#23434 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AV7DMHLqF7T2mhEFJ1P2_rzeuiM4RFeQks5uqbnIgaJpZM4X21U6> .

Re-landing #23412 with a fix to the PlatformLayer's addToScene signature. #23412 was broken by the change to Layer done in #23434. It seemed green as the presubmits were done before #23434 was landed, and when #23412 landed it broke the build. Reverts #23779

For flutter#21756

…23781) Re-landing flutter#23412 with a fix to the PlatformLayer's addToScene signature. flutter#23412 was broken by the change to Layer done in flutter#23434. It seemed green as the presubmits were done before flutter#23434 was landed, and when flutter#23412 landed it broke the build. Reverts flutter#23779

liyuqian added 3 commits October 23, 2018 13:13

Override isDirty instead of markClean

49925c1

Improve documentations

7b6380f

liyuqian requested a review from Hixie October 23, 2018 23:13

googlebot added the cla: yes label Oct 23, 2018

Hixie reviewed Oct 24, 2018

View reviewed changes

liyuqian mentioned this pull request Oct 24, 2018

Retained rendering in Fuchsia PhysicalShapeLayer flutter-team-archive/engine#6558

Merged

liyuqian added 2 commits October 24, 2018 16:25

Changes before adding getters and setters

703eea0

Mark layer fields as final, or set needsAddToScene

a11a601

liyuqian commented Oct 24, 2018

View reviewed changes

liyuqian added 3 commits October 24, 2018 16:47

Solve analyze issue by using required

b54f915

Revert final field change and add getter/setter

9271e96

Better documentation and 2 more getters/setters.

8219ad2

Hixie reviewed Oct 25, 2018

View reviewed changes

Nits

a0322b3

liyuqian merged commit 883e2dc into flutter:master Oct 25, 2018

liyuqian mentioned this pull request Oct 25, 2018

Enable Retained Rendering #21756

Closed

zanderso reviewed Oct 26, 2018

View reviewed changes

amirh mentioned this pull request Oct 31, 2018

Re-land "Initial framework support for iOS platform views." #23781

Merged

Xavjer pushed a commit to Xavjer/flutter that referenced this pull request Nov 1, 2018

Maintain dirtiness and use retained engine layers (flutter#23434)

b199b9b

For flutter#21756

github-actions Bot locked as resolved and limited conversation to collaborators Aug 9, 2021

		@@ -168,8 +229,9 @@ class PictureLayer extends Layer {
		bool willChangeHint = false;

Conversation

liyuqian commented Oct 23, 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!

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!

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!

Hixie commented Oct 24, 2018

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!

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!