Release retained resources from layers/dispose pictures by dnfield · Pull Request #84740 · flutter/flutter

dnfield · 2021-06-16T21:16:08Z

This implements Layer.dispose, and makes sure that the RenderObject.layer and PaintingContext._currentLayer get properly disposed when render objects are done with them.

It also adds some affordances for managing layer disposal when a render object needs an additional layer, which is a relatively common case for a repaint boundary that also needs to do some clipping.

This is the last major user-facing piece of https://flutter.dev/go/picture-lifecycle, however there is some additional work planned beyond this. Specifically, flutter/engine#26375 can still help developers catch leaks (including framework/engine developers), and flutter/engine#26870 will further reduce GC related activity that is no longer as essential since we more eagerly release native resources after this patch.

Locally, this patch is showing significant boosts in transition perf benchmarks. I believe this is related to reduced GC pressure and reduced need to perform GCs.

/cc @knopp @flar - I think this will be helpful with some of the diff context work
/cc @yjbanov since we've been discussing this for web
/cc @jacob314 - mistakes around this are the kind of thing we'll want to catch with the work I've been discussing about GC-but-not-disposed objects.
/cc @goderbauer @Hixie @zanderso

fixes #81514

dnfield · 2021-06-21T23:25:57Z

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

+///
+/// This class manages automatically creation and diposing of layer handles for
+/// its associated [layer].
+class LayerHolder<T extends Layer?> {


reviewers: I'm somewhat tempted to make this a mixin on RenderObject so it can override and handle dispose for you. The problem is that it would mean a RenderObject could only easily handle one additional layer, and we'd have to give the layer a weird property name like additionalLayer or something.

The framework only needs one additional layer for any of the RenderObjects that tests have picked up, but I'm a bit put off by the naming weirdness and the odd restriction. I'm open to ideas though!

What if LayerHandle instead of a dispose method rendering it unusable simply had a set layer that accepted null? Then the LayerHandle wouldn't need to be nulled out in various dispose methods, and we wouldn't need LayerHolder?

Strawman:

class LayerHandle { set layer(Layer? layer) { if (identical(layer, _layer)) return; _layer._unref(); _layer = layer; } Layer get layer => _layer!; }

Instead of this:

child._parentHandle!.dispose(); child._parentHandle = null;

One would do:

child._parentHandle.layer = null;

Render objects could use LayerHandle and LayerHolder wouldn't be necessary.

I've more or less done this, but made it Layer? get layer => _layer;.

Hixie · 2021-06-22T00:07:08Z

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

  }

+  /// Used for tests that directly invoke [RenderObject.paint] or
+  /// [RenderObject.debugPaint].


this documentation should still say what it does (including what it does in release builds)

Done. Also updated docs on the canvas getter to explain what tests might need to do if they directly invoke paint/debugPaint.

dnfield · 2021-06-22T03:25:23Z

/cc @ds84182 - this breaks a render object you created internally. I'll likely have to introduce an intermediate step here with some changes to the Layer interface that don't do/assert anything yet, fix up _FocusOverlay, and then re-introduce the asserts - but happy to discuss further with you if you have a better idea.

dnfield · 2021-06-22T15:38:31Z

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

+        'Do not directly call dispose on a $runtimeType. Instead, '
+        'use createHandle and LayerHandle.dispose.',
+      );
+      _debugDisposed = true;


Commenting out this line is enough to make internal tests pass (since it undoes other assertions that you avoid using a layer if it is disposed).

I think it might be a good idea to do that until the internal RO that needs updating is updated, with a TODO to enable this.

Layers are technically usable after disposal, but triggering this assert means you failed to hold a reference to a layer that you wanted to keep alive, and its native resources are gone. Alternatively, it means that you did want to dispose that RO and failed to dispose it when you should have, meaning you held native resources longer than you needed to.

@ds84182 FYI

dnfield · 2021-06-22T20:18:07Z

This patch now passes internally.

yjbanov · 2021-06-23T19:53:09Z

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

+      count = _refCount;
+      return true;
+    }());
+    return count;


This will throw an error complaining that count hasn't been initialized. Can we instead throw something that talks about debuHandleCount being only supported in debug mode?

I'm pretty sure this is how we handle these sorts of debug* elsewhere. I'd really just rather leave this method as sparse as possible.

yjbanov · 2021-06-23T19:54:29Z

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

+  int _refCount = 0;
+
+  void _unref() {
+    assert(_refCount > 0);


Should we attach a message to this assert?

This would have to be a bug in the framework, if someone broke either createHandle or LayerHandle.dispose. End users should never see this, and if they somehow do they don't have a way to fix it short of patching the framework/filing a bug. Without a message here, they'll get a message encouraging them to file a bug with the stack trace/reproduction.

yjbanov · 2021-06-23T19:56:38Z

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

+  @protected
+  @visibleForTesting
+  void dispose() {
+    assert(!_debugDisposed);


Should we attach a message to this assert?

yjbanov · 2021-06-23T19:57:48Z

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

+        'use createHandle and LayerHandle.dispose.',
+      );
+      // TODO(dnfield): enable this. https://github.com/flutter/flutter/issues/85066
+      // _debugDisposed = true;


Can we print a warning to nudge people to fix the lifecycles of their objects?

We could, but that'd mean commenting out more asserts elsewhere instead of this one.

There's only one customer that is actually affected by this in all of customer_testing/google3. I don't plan to leave this commented out for long.

yjbanov · 2021-06-23T21:04:00Z

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

+/// the handles for that layer similarly to the implementation of
+/// [RenderObject.layer].
+///
+/// Layers can be reused even after their native resources has been released,


nit: s/has/have/

Ah, this whole paragraph is obsolete. Removed it.

yjbanov · 2021-06-23T21:06:44Z

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

+
 /// A composited layer containing a [Picture].
 ///
 /// Picture layers are always leaves in the layer tree.


Since this class is responsible for calling Picture.dispose it is effectively the owner of the Picture object it holds. We should note that in the dartdocs.

yjbanov · 2021-06-23T21:08:42Z

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

+
+/// A utility for managing a layer maintaining a handle to it.
+///
+/// This class manages automatically creation and diposing of layer handles for


typo: diposing

yjbanov · 2021-06-23T21:13:27Z

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

+///
+/// This class manages automatically creation and diposing of layer handles for
+/// its associated [layer].
+class LayerHolder<T extends Layer?> {


What if LayerHandle instead of a dispose method rendering it unusable simply had a set layer that accepted null? Then the LayerHandle wouldn't need to be nulled out in various dispose methods, and we wouldn't need LayerHolder?

Strawman:

class LayerHandle { set layer(Layer? layer) { if (identical(layer, _layer)) return; _layer._unref(); _layer = layer; } Layer get layer => _layer!; }

Instead of this:

child._parentHandle!.dispose(); child._parentHandle = null;

One would do:

child._parentHandle.layer = null;

Render objects could use LayerHandle and LayerHolder wouldn't be necessary.

yjbanov · 2021-06-23T23:18:37Z

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

+  /// [RenderObject.debugPaint].
+  ///
+  /// When accessing the [canvas], the context expects that it has started the
+  /// painted operation on the render object via [paintChild] or


nit: "paint operation"?

yjbanov · 2021-06-23T23:19:33Z

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

  /// context, which means it's fragile to hold a reference to the canvas
  /// returned by this getter.
+  ///
+  /// This getter requires that the current [RenderObject] being registered for


nit: s/being/is/

yjbanov · 2021-06-23T23:26:15Z

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

  ContainerLayer? get layer {
-    assert(!isRepaintBoundary || (_layer == null || _layer is OffsetLayer));
-    return _layer;
+    assert(!isRepaintBoundary || (_layerHandle.layer == null || _layerHandle.layer is OffsetLayer));


nit: redundant parens, I think

yjbanov · 2021-06-23T23:29:06Z

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

  }

+  final LayerHandle<ContainerLayer> _layerHandle = LayerHandle<ContainerLayer>();
+  final List<LayerHandle<PictureLayer>> _pictureLayerHandles = <LayerHandle<PictureLayer>>[];


Why do render objects need to maintain handles to pictures? It seems as if this is needed to prolong the lifetimes of pictures, but I don't see these handles used for anything, so I'm not sure why we'd want to prolong their lifetimes. If a picture layer is still used by a ContainerLayer (transitively from some render object), then that container layer would keep a handle making sure that the picture is not collected. Is there a use-case I'm missing?

This is a good catch.

I wrote this code first before working out how to handle refcounting on ContainerLayers, and some of my experiments with trying to do that were trying to dispose picture layers at the wrong time and causing problems. Having the RenderObjects do it was working.

However, that caused problems for repaint boundaries. When I finally worked out the attach/append/detach/remove distinction, that problem went away but I didn't revisit this code.

I'm removing this code and the associated debug* field/methods for it. All tests are still passing with it, and apps I'm testing with locally still correctly release memory without it.

yjbanov · 2021-06-23T23:36:06Z

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

@@ -2056,8 +2108,8 @@ abstract class RenderObject extends AbstractNode with DiagnosticableTreeMixin im
  /// field.


This doc comment may be implying things that are no longer correct. For example, "It is also OK to leave this field as null and create a new layer on every repaint, but without the performance benefit" is probably not true. If the field is not populated we may end up leaking memory, no?

I'll update this to mention that you should use a LayerHandle so that the layer gets appropriately disposed.

The real problem is if the RO tries to hold another layer in a private field and then reuse it.

yjbanov · 2021-06-23T23:59:56Z

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

+    }());
+    return object;
+  }
+  static RenderObject? _activePaint;


I missed this in the previous PR but would it make more sense to put this field on the PipelineOwner instead of making it a static?

I've reverted this change since it's not needed anymore - this has gone back to being a debug static thing.

yjbanov

yjbanov · 2021-06-25T16:42:01Z

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

 /// a layer for later usage, it must create a handle to that layer. This is
 /// handled automatically for the [RenderObject.layer] property, but additional
-/// layers should be handled with a [LayerHandle] class.
+/// layers must user their own [LayerHandle].


nit: s/user/use/

yjbanov · 2021-06-25T16:49:58Z

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

+/// layer in [RenderObject.paint], it should dispose of the handle to the
+/// old layer. It should also dispose of any layer handles it holds in
+/// [RenderObject.dispose].
+class LayerHandle<T extends Layer?> {


I think this can be <T extends Layer> (i.e. the non-null upper bound). This is because internally T? will automatically allow null values, even if the layer type used by LayerHandle is not nullable.

Ahh ok. This works if you don't use LayerHandle<Layer?> anywhere else.

yjbanov · 2021-06-25T16:51:05Z

packages/flutter/lib/src/cupertino/text_selection_toolbar.dart

  }

-  ClipPathLayer? _clipPathLayer;
+  final LayerHandle<ClipPathLayer?> _clipPathLayer = LayerHandle<ClipPathLayer?>();


I think this can be LayerHandle<ClipPathLayer> (i.e. no ?). null will still be allowed because LayerHandle class specifies T?.

yjbanov · 2021-06-25T16:54:10Z

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

      _paintWithDelegate(context, offset);
    } else {
-      _clipRectLayer = context.pushClipRect(
+      _clipRectLayer.layer = context.pushClipRect(


I know this would be a significant breaking change, but it would be nice if all context.push* methods would just take LayerHandle as an argument. Then they could be void because they could populate the handle internally (sort of like a ref parameter in C#):

context.pushClipRect(..., _clipRectLayerHandle); // ta-da!

yeah, this would be nice. It may make sense to make some pushHandle methods and deprecate the existing ones.

yjbanov · 2021-06-25T17:04:20Z

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

+  /// imply that it has been removed from its parent.
+  final LayerHandle<Layer> _parentHandle = LayerHandle<Layer>();
+
+  /// Incremenetd by [LayerHandle].


typo in "Incremenetd"

yjbanov · 2021-06-25T17:18:33Z

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

+///
+/// A handle is also automatically managed for [RenderObject.layer].
+///
+/// If a [RenderObject] creates layers in addition to its [RenderObject.layer],


"... and it intends to reuse those layers separately from [RenderObject.layer]" (a handle is not required if the additional layers are disposed of simultaneously with RenderObject.layer; the layers will be removed when the layer tree is disassembled).

yjbanov · 2021-06-25T17:19:21Z

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

+/// old layer. It should also dispose of any layer handles it holds in
+/// [RenderObject.dispose].
+class LayerHandle<T extends Layer?> {
+  /// Creates a new LayerHandle, optionally referencing a [Layer].


nit: I'd use plain English "Create a new layer handle".

yjbanov · 2021-06-25T17:21:24Z

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

+
+  T? _layer;
+
+  /// The [Layer] this class holds.


nit: "The [Layer] whose resources this handle keeps alive."?

yjbanov · 2021-06-25T17:22:11Z

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

+  /// Setting a new value will dispose the previously held layer if there are
+  /// no other open handles to that layer.
+  ///
+  /// To dispose of this handle, set this value to `null`.


nit: instead of an extra paragraph specifically for null, we could say "Setting a new value or null will dispose..." in the previous paragraph. It would make it clear that nulling out the layer does not guarantee disposal, that all other handles must also be cleared before the layer is disposed of.

yjbanov · 2021-06-25T17:25:21Z

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

 ///
-/// Picture layers are always leaves in the layer tree.
+/// Picture layers are always leaves in the layer tree. They are also
+/// responsible for disposing of the [Picture] object they hold. This is done


nit: "This is typically done" (in case some users don't use the upper layers of the framework, e.g. perhaps they have their own custom render object system)

dnfield · 2021-06-25T18:00:33Z

Thanks @yjbanov - addressed all the nits.

goderbauer · 2021-07-07T19:13:21Z

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

+
+  /// The [Layer] whose resources this object keeps alive.
+  ///
+  /// Setting a new value will or null dispose the previously held layer if


typo: "will" should probably come after "or null"?

Yeah, I'll fix this.

flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label Jun 16, 2021

google-cla bot added the cla: yes label Jun 16, 2021

dnfield mentioned this pull request Jun 16, 2021

Picture disposal - work in progress #83283

Closed

This comment has been minimized.

Sign in to view

dnfield force-pushed the layer_retain branch from e1c00e4 to 1c85c10 Compare June 16, 2021 21:48

This comment has been minimized.

Sign in to view

dnfield mentioned this pull request Jun 21, 2021

canvas.drawPicture consumes a lot of memory and freezes the app #84571

Closed

Release retained layer resources

81571c8

dnfield force-pushed the layer_retain branch from b37af74 to 81571c8 Compare June 21, 2021 19:16

Rationalize dispose;

ce1adba

dnfield commented Jun 21, 2021

View reviewed changes

dnfield mentioned this pull request Jun 21, 2021

Remove tech debt related to image disposal and layer GC flutter/engine#26870

Merged

1 task

flutter-dashboard bot added f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. labels Jun 21, 2021

merge

9a92be8

Hixie reviewed Jun 22, 2021

View reviewed changes

dnfield force-pushed the layer_retain branch 2 times, most recently from e4172c9 to da6f798 Compare June 22, 2021 03:48

dnfield added 2 commits June 21, 2021 20:51

docs

ae0d837

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

d61a65a

dnfield force-pushed the layer_retain branch from da6f798 to d61a65a Compare June 22, 2021 03:52

dnfield commented Jun 22, 2021

View reviewed changes

This was referenced Jun 22, 2021

Avoid calling Dart_HintFreed more than once every five seconds flutter/engine#25827

Merged

Assert that disposed layers are not re-used #85066

Closed

Update layer.dart

da94429

dnfield marked this pull request as ready for review June 22, 2021 20:17

dnfield requested review from goderbauer and knopp June 22, 2021 20:23

Update layer.dart

ffe710e

dnfield requested a review from yjbanov June 22, 2021 21:14

skips

2562969

yjbanov reviewed Jun 23, 2021

View reviewed changes

dnfield added 2 commits June 23, 2021 15:04

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

f552f7a

LayerHolder -> LayerHandle

aff3cfc

yjbanov reviewed Jun 24, 2021

View reviewed changes

dnfield added 2 commits June 23, 2021 18:47

remove picturelayer refs

e0a06aa

remove unused method, fix docs

3727088

yjbanov approved these changes Jun 25, 2021

View reviewed changes

merge

c95064a

review

b5573dd

dnfield added the waiting for tree to go green label Jun 25, 2021

fluttergithubbot merged commit 025397a into flutter:master Jun 25, 2021

dnfield deleted the layer_retain branch July 2, 2021 17:30

goderbauer reviewed Jul 7, 2021

View reviewed changes

		@@ -2056,8 +2108,8 @@ abstract class RenderObject extends AbstractNode with DiagnosticableTreeMixin im
		/// field.

Conversation

dnfield commented Jun 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

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!

dnfield commented Jun 22, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnfield commented Jun 22, 2021

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!

Choose a reason for hiding this comment

Uh oh!

dnfield commented Jun 16, 2021 •

edited

Loading