Add a mechanism to observe layer tree composition. by dnfield · Pull Request #103378 · flutter/flutter

dnfield · 2022-05-10T00:09:03Z

This is WIP to enable a refactor of visibility_detector that does not affect compositing.

Currently the way visibility_detector works is use a render object that always requires composition so that it can push a layer that always re-composites its part of the tree so that it can observe composition.

This has several major drawbacks:

It defeats raster caching for parts of the tree that would otherwise be cacheable
It marks parts of the tree as dirty more often
It increases the total size of the layer tree
It breaks up portions of the tree surrounded by these layers into individual pictures, which has negative performance impacts in most cases

This patch does not have tests yet, and in particular it's not clear to me whether we'd need some kind of removeCallback mechanism - if we do, the API will have to look different.

Looking for some early feedback while I continue to work on the visibility_detector change.

dnfield · 2022-05-10T00:24:32Z

removeCallback case: call it when RO gets disposed.

Argument against: RO can track when it's disposed and just short circuit the callback. RO also shoul dnever really get disposed when its tree is compositing anyway...?

jonahwilliams · 2022-05-10T01:10:48Z

packages/flutter/lib/src/rendering/layer.dart


  void _fireCompositionCallbacks(bool includeChildren) {
-    for (final VoidCallback callback in List<VoidCallback>.of(_callbacks)) {
+    for (final VoidCallback callback in List<VoidCallback>.of(_callbacks.values)) {


I think .toList is the fast one since the exact type of the receiver is known in the implementation

toList calls of

🤦‍♂️

https://github.com/dart-lang/sdk/blob/main/sdk/lib/_internal/vm/lib/array.dart#L169

Looks like if you give List.of an iterable that isn't backed by a list it first converts it to a list. so calling toList should be universally better

https://github.com/dart-lang/sdk/blob/8aeeffb84825fa91c8b270afe2cdf77fad1cf1d5/sdk/lib/core/iterable.dart#L469-L471

goderbauer · 2022-05-10T16:07:22Z

packages/flutter/lib/src/rendering/object.dart

+  /// Adds a [CompositionCallback] for the current [ContainerLayer] used by this
+  /// context.
+  ///
+  /// Composition callbacks are called whenever the layer tree a container layer


nit: this sentence is missing an is, I think.

Reworded - I changed my mind on how this was going to sound and forgot to finish rewriting it ahah

goderbauer · 2022-05-10T16:07:54Z

packages/flutter/lib/src/rendering/object.dart

+  ///
+  /// Calling the returned callback will remove [callback] from the composition
+  /// callbacks.
+  /// See also:


nit: add blank line before this

goderbauer · 2022-05-10T16:10:21Z

packages/flutter/lib/src/rendering/layer.dart

+      child._fireCompositionCallbacks(includeChildren);
+      child = child.nextSibling;
+    }
+


nit: remove blank line

goderbauer · 2022-05-10T16:11:51Z

packages/flutter/lib/src/rendering/layer.dart

 abstract class Layer extends AbstractNode with DiagnosticableTreeMixin {
+  final Map<int, VoidCallback> _callbacks = <int, VoidCallback>{};
+
+  void _fireCompositionCallbacks(bool includeChildren) {


nit: make the argument a required optional parameter to make callsites easier to read.

goderbauer · 2022-05-10T16:13:17Z

packages/flutter/lib/src/rendering/layer.dart

+
+  }
+
+  /// Adds a callback for when the layer tree that this layer is part of gets


Conceptually, why is this a feature of ContainerLayer over the Layer base class?

Mainly because RenderObjects are the normal clients for this via the PaintingContext, and the PaintingContext only knows its container layer - picture layers might not ever be created for it.

Since the backing storage is in the Layer class, it feels like this method should also be defined there? PaintingContext could still use it on the ContainerLayer, right?

That will then require casts to get to parents, but I guess that's ok.

oh, it won't. I see now.

It will but it's probably ok.

goderbauer · 2022-05-10T16:17:11Z

packages/flutter/lib/src/rendering/layer.dart

      child = child.nextSibling;
    }
+    // Children fired them already in child.detach().
+    _fireCompositionCallbacks(false);


Why do we need to fire on detach?

Detach indicates we're not going to get drawn again. If we don't fire here, we risk telling clients that we rendered something and never updating when we got dropped.

Attach shouldn't be needed because if you get composed after attach buildScene will be called.

I'll update the comment.

Maybe also update the doc comment explaining when the callback will be called. From the current description my understanding was it would only happen when the layer is still part of the scene, not when it is going away...

goderbauer · 2022-05-10T16:21:24Z

packages/flutter/lib/src/rendering/layer.dart

  ui.Scene buildScene(ui.SceneBuilder builder) {
    updateSubtreeNeedsAddToScene();
    addToScene(builder);
+    _fireCompositionCallbacks(true);


This adds a walk of the entire layer tree every time we build a new scene (i.e. every single frame) - irregardless of whether any callbacks are registered or not and people not using this feature at all would also pay the cost?

So one thought I had is that we could mark the root of a tree about whether any callbacks are actually present or not. I started working on that but abandoned it because of added complexity and I just wanted to have something working. I can take a look at it again.

Ideally if there are no callbacks this has zero cost. maybe we do that with a single pristine check for now and optimize later?

goderbauer

Overall, I think this looks reasonable.

goderbauer · 2022-05-10T20:44:15Z

packages/flutter/lib/src/rendering/layer.dart

      markNeedsAddToScene();
    }
+
+    _incrementSubtreeCompositionObserverCount(-(child as Layer)._childrenWithCompositionCallbacks);


We can probably type the "child" argument as Layer in the method signature, right?

I think that will cause breakage in implementations that override these methods today, but it's probably worth it...

goderbauer · 2022-05-10T20:44:21Z

packages/flutter/lib/src/rendering/layer.dart

    if (!alwaysNeedsAddToScene) {
      markNeedsAddToScene();
    }
+    _incrementSubtreeCompositionObserverCount((child as Layer)._childrenWithCompositionCallbacks);


goderbauer · 2022-05-10T20:47:00Z

packages/flutter/lib/src/rendering/layer.dart

+    }
+  }
+
+  /// Adds a callback for when the layer tree that this layer is part of gets


it also gets called when the layer is no longer part of the tree, right?

goderbauer · 2022-05-10T20:47:55Z

packages/flutter/lib/src/rendering/layer.dart

+
+  }
+
+  /// Adds a callback for when the layer tree that this layer is part of gets


Since the backing storage is in the Layer class, it feels like this method should also be defined there? PaintingContext could still use it on the ContainerLayer, right?

goderbauer · 2022-05-10T20:49:32Z

packages/flutter/lib/src/rendering/layer.dart

+    return () {
+      _callbacks.remove(_nextCallbackId);
+      if (_callbacks.isEmpty) {
+        _incrementSubtreeCompositionObserverCount(-1);


nit: maybe this should be called update instead of increment?

goderbauer · 2022-05-10T20:50:43Z

packages/flutter/lib/src/rendering/layer.dart

+    // interested in observing composition will need to get updated to show
+    // Children fired them already in child.detach().


something is odd here. Is the sentence on the previous line incomplete?

Yes, fixed that.

goderbauer · 2022-05-10T20:52:50Z

packages/flutter/lib/src/rendering/layer.dart

+  ///
+  /// The callback receives a reference to this layer. The recipient must not
+  /// mutate the layer during the scope of the callback, but may traverse the
+  /// tree to find information about the current transform or clip.


We should probably document what happens if new callbacks are added/removed within the callback.

goderbauer · 2022-05-10T20:54:32Z

packages/flutter/lib/src/rendering/layer.dart

+  /// layers.
+  ///
+  /// The callback receives a reference to this layer. The recipient must not
+  /// mutate the layer during the scope of the callback, but may traverse the


nit: if the callback is received do to a detach there will be nothing to traverse. Somewhere this doc should talk about that case.

Detached != not in a tree, it just means there's no owner.

You can be in a state where !attached && parent != null.

flutter-dashboard · 2022-05-10T23:40:56Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

dnfield · 2022-05-10T23:41:02Z

Added some tests.

goderbauer · 2022-05-11T02:46:36Z

packages/flutter/lib/src/rendering/layer.dart

      markNeedsAddToScene();
    }
+    if (child._childrenWithCompositionCallbacks != 0) {
+      _updateSubtreeCompositionObserverCount(child._childrenWithCompositionCallbacks);


isn't this missing a -? The count needs to be reduced by child._childrenWithCompositionCallbacks right?

If there is no failing test for this case, let's add one! :)

You are right. I thought I had a test covering this but did not. Added one.

goderbauer · 2022-05-16T23:59:36Z

packages/flutter/lib/src/rendering/layer.dart

+        _debugMutationsLocked = true;
+        return true;
+      }());
+      callback(this is ContainerLayer ? this as ContainerLayer : parent!);


Why is this not just callback(this)?

From the docs, that's what I would have expected here and the type checking is kinda odd here. Also, what's the guarantee that a non-Container Layer will always have a non-null parent?

The callback should probably just always receive a reference to "this" - if the callback wants, it can then check whether it was given a non-ContainerLayer and access the parent.

The callback was written to receive a ContainerLayer, which has all the interesting methods for traversal on it.

In the layer tree, leaf Layer objects always have a parent at compositing time - in fact, this method is called from buildScene, which only exists on ContainerLayer. If there was no container layer parent, then this method would never get called.

If the callback was typed to receive a Layer, for example, and a caller wanted to start reviewing the clip bounds applied, it woud have to do something like ContainerLayer container = layer is ContainerLayer ? layer : layer.parent!.

I'll ggo ahead and change this now - I suppose if the implementation ever shifted or some new more interesting method for this is added on Layer it'd be better to not have to refactor this API later.

Also moved describeClipBounds to layer.

goderbauer · 2022-05-17T00:00:30Z

packages/flutter/lib/src/rendering/layer.dart

 }

+/// The signature of the callback added in [Layer.addCompositionCallback].
+typedef CompositionCallback = void Function(ContainerLayer);


Related to the above, why is the argument not typed as ContainerLayer and not just Layer?

goderbauer · 2022-05-17T00:05:55Z

packages/flutter/lib/src/rendering/object.dart

+  /// Adds a [CompositionCallback] for the current [ContainerLayer] used by this
+  /// context.
+  ///
+  /// Composition callbacks are called whenever the layer tree containing the


nit: some of these docs seem duplicated from Layer.addCompositionCallback. Consider using a macro.

goderbauer · 2022-05-17T16:14:21Z

packages/flutter/lib/src/rendering/layer.dart

+    };
+    return () {
+      _callbacks.remove(callbackId);
+      if (_callbacks.isEmpty) {


This seems incorrect? We are increasing the count unconditionally in line 203 whenever a new callback is added, but we are only decreasing the count by one when the last callback is removed. So if I add 2 and remove both of them the _childrenWithCompositionCallbacks would remain at 1 + 1 - 1 = 1?

Not sure what the desired behavior for the count is. _updateSubtreeCompositionObserverCount makes it sound like we want to count observers, so increasing for each callback sounds right. But _childrenWithCompositionCallbacks makes it sound like we are counting the children that have observers meaning we should only increase the count for the first observer added to a child. We should probably unify the naming...

Ahh yes, I mixed up two different approaches I was trying. I'll write some more tests for this and fix.

Added a test, unconditionally call -1 here, and renamed var.

goderbauer

LGTM

goderbauer · 2022-05-17T16:41:47Z

packages/flutter/lib/src/rendering/layer.dart

+      }());
+    };
+    return () {
+      _callbacks.remove(callbackId);


Maybe assert that the callback is actually still in _callbacks before attempting to remove it to avoid that somebody accidentally calls the remover twice?

Done.

Calling it twice is wasted work but it shouldn't cause any program incorrectness at least. But assert sgtm.

…lutter#103378)

Add a mechanism to observe layer tree composition.

3d67a2a

dnfield requested review from goderbauer and jonahwilliams May 10, 2022 00:09

flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label May 10, 2022

Call in detach, add cancellation mechanism

1f28606

jonahwilliams reviewed May 10, 2022

View reviewed changes

dnfield added 2 commits May 9, 2022 21:43

Doc typos

c403c47

Fire even if root layer, reduce complexity

2b958cb

goderbauer reviewed May 10, 2022

View reviewed changes

dnfield mentioned this pull request May 10, 2022

Refactor visibility_detector to avoid forcing compositing. google/flutter.widgets#367

Merged

goderbauer review

68482a2

goderbauer reviewed May 10, 2022

View reviewed changes

review and tests

3011c2c

dnfield marked this pull request as ready for review May 10, 2022 23:40

goderbauer reviewed May 11, 2022

View reviewed changes

dnfield added 4 commits May 10, 2022 20:15

- and test

439c7e6

skip addretained on web

7842959

Unskip

9785d04

Merge remote-tracking branch 'upstream/master' into spy_layer

fbf9bcd

dnfield mentioned this pull request May 12, 2022

Let render objects know when their repaint boundary is moved #103156

Closed

goderbauer reviewed May 17, 2022

View reviewed changes

dnfield added 4 commits May 16, 2022 20:42

Merge remote-tracking branch 'upstream/master' into spy_layer

6e76b56

Macro

09cf1ba

Layer

66ff095

move describeClipBounds to Layer

428ba11

goderbauer reviewed May 17, 2022

View reviewed changes

test, fix

d529386

dnfield added 2 commits May 17, 2022 09:33

Merge remote-tracking branch 'upstream/master' into spy_layer

3832c09

rename var

eae56b9

goderbauer approved these changes May 17, 2022

View reviewed changes

assert

d725f10

dnfield added the waiting for tree to go green label May 17, 2022

fluttergithubbot merged commit d54cdf9 into flutter:master May 17, 2022

engine-flutter-autoroll mentioned this pull request May 17, 2022

Roll Flutter from 1994027986cf to 9651f1d7171b (7 revisions) flutter/packages#1953

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 17, 2022

d54cdf9 Add a mechanism to observe layer tree composition. (flutter/f…

a53a0a8

…lutter#103378)

engine-flutter-autoroll mentioned this pull request May 18, 2022

Roll Flutter from 1994027986cf to 6eb65b376d82 (30 revisions) flutter/plugins#5781

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request May 18, 2022

d54cdf9 Add a mechanism to observe layer tree composition. (flutter/f…

15d420f

…lutter#103378)

engine-flutter-autoroll mentioned this pull request May 18, 2022

Roll Flutter from 1994027986cf to a4a8e73bce15 (31 revisions) flutter/plugins#5782

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request May 18, 2022

d54cdf9 Add a mechanism to observe layer tree composition. (flutter/f…

96c1ece

…lutter#103378)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 30, 2022

d54cdf9 Add a mechanism to observe layer tree composition. (flutter/f…

e8b9178

…lutter#103378)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Aug 30, 2022

d54cdf9 Add a mechanism to observe layer tree composition. (flutter/f…

a1218bc

…lutter#103378)


		}

		/// Adds a callback for when the layer tree that this layer is part of gets

		// interested in observing composition will need to get updated to show
		// Children fired them already in child.detach().

Comments

Conversation

dnfield commented May 10, 2022

Uh oh!

dnfield commented May 10, 2022

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!

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!

goderbauer 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