Dispose of images after using them by dnfield · Pull Request #66688 · flutter/flutter

dnfield · 2020-09-25T21:53:24Z

Description

This patch should close the image disposal saga.

Does the following:

Adds ImageInfo.clone and ImageInfo.isCloneOf.
Makes all stateful holders of ui.Image objects properly clone and dispose of them.
Behavior change for ImageStreamCompleter: Removing the last listener after registering at least one listener results in the object being disposed, and new listeners cannot be added.
Allow ImageStreamCompleter to have "passive" listeners, which are listeners that do not drive frames forward but keep the stream alive and get updates if there are other "active" listeners. For example, the image cache listens to the stream this way.
Makes all non-stateful holders of ui.Image (i.e. paintImage and RawImage) assert that whoever gave the image to them has not disposed it on them.

Related Issues

Fixes #56482 for real this time, by only triggering additional GCs if we've disposed an image large enough to get the VM's attention.

A previous, rejected attempt here: #64582
Related engine patches:

flutter/engine#21057 (adds Image.clone
flutter/engine#20746 (an overly aggressive attempt to use Dart_HintFreed to achieve this goal, got reverted).
flutter/engine#20754 (uses HintFreed specifically in CanvasImage::dispose)

Tests

I added the following tests:

A bunch of tests for image, image cache, image stream, image stream completer, to make sure we dispose of the right things at the right times.

There is also a devicelab memory benchmark for this use case that was added in #64762

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

No, no existing tests failed, so this is not a breaking change.

dnfield · 2020-09-25T22:28:58Z

Apparently this broke something in gallery - marking as draft while I figure that out.

dnfield · 2020-09-25T22:49:06Z

It's a bug in dart:ui. Fix will be posted shortly.

dnfield · 2020-09-25T22:53:19Z

Blocked by flutter/engine#21422

dnfield · 2020-09-28T18:57:48Z

Engine fix has been rolled in, this is ready for review again.

dnfield · 2020-09-28T20:51:52Z

Ok, tests area ll passing now :)

jonahwilliams · 2020-09-28T20:56:13Z

packages/flutter/lib/src/painting/image_stream.dart

 /// ImageInfo objects are used by [ImageStream] objects to represent the
 /// actual data of the image once it has been obtained.
+///
+/// If you receive an [ImageInfo] object, it is your responsibility to call


Who is you? Prefer a bit of passive voice instead maybe?

jonahwilliams · 2020-09-28T21:01:47Z

packages/flutter/lib/src/painting/image_stream.dart

    : assert(image != null),
      assert(scale != null);

+  /// Creates an [ImageInfo] with a cloned [image].


when would someone want/need to create a clone?

jonahwilliams · 2020-09-28T21:01:57Z

packages/flutter/lib/src/painting/image_stream.dart

+
+  /// Whether this [ImageInfo] is a [clone] of the `other`.
+  ///
+  ///


Why is comparing clones useful?

Expanded with example.

jonahwilliams · 2020-09-28T21:21:26Z

packages/flutter/lib/src/painting/image_stream.dart

+  /// own image reference, it can no longer access the image, but other clients
+  /// will be able to access their own references.
+  ///
+  /// This method must be used in cases where a client holding an [ImageInfo]


concrete example of a Client is an Image widget? Or an image cache?

Any method or class receiving this object.

jonahwilliams · 2020-09-28T21:26:48Z

packages/flutter/lib/src/widgets/basic.dart

+    );
    renderObject
-      ..image = image
+      ..image = image?.clone()


why does this clone if it doesn't dispose? won't this create extra image clones

The render object disposes of it. This can't dispose because it's stateless - it needs to be kept alive by whatever created RawImage.

This is pretty confusing - so the State class has its copy, which it will clone/dispose as needed. Then the RawImage is just a pass through, and finally the RO accepts the clone and does a dispose.

Maybe a safer arrangement would be for the RO to clone in the setter, so that the behavior is internal to it?

I'm trying to keep the contract as you should never have to clone something you received. If we have some places that clone immediately I think it's more confusing.

However, I agree that RawImage is a sore spot, and I think it'd be preferable to just make it private since it's easy to misuse.

I have also been confused by the fact that cloning and disposing are done by separate entities.

Yeah, if it needs to act like an implementation detail of the Image class it should be private. But we do have other RawFoo classes exposed in the framework so that people could build on top of that, though I think there is also a rendering helper method too?

I dunno.

Refactored RawImage to not be a special case. There's a private impl that's a special case now though.

I had to revert that refactor because it doesn't work to have the image hold a stateful object like that.

why not have the widget pass through, and its caller (the call to the RawImage constructor) have to call clone()?

The widget might create multiple RenderObjects (via multiple elements). It has no defined lifetime, and so it's not clear when the widget's clone would ever get disposed.

goderbauer · 2020-09-28T21:36:57Z

packages/flutter/lib/src/painting/image_cache.dart

-      image.completer.addOnLastListenerRemovedCallback(image.handleRemove);
-      return image;
-    }).sizeBytes ??= image.sizeBytes;
+      return _LiveImage(


The comment above here is confusing and/or outdated since no listener is added in the code below?

Updated the comment.

goderbauer · 2020-09-28T21:44:56Z

packages/flutter/lib/src/painting/image_stream.dart

+    _processNewListener(listener);
+  }
+
+  /// Adds a listener callback that is called only called if there are listeners


grammer: ... is called only called if ...

goderbauer · 2020-09-28T21:46:12Z

packages/flutter/lib/src/painting/image_stream.dart

+  /// object is available or an error is reported. If a concrete image is
+  /// already available, or if an error has been already reported, this object
+  /// will notify the listener synchronously.
+  ///


This should document that listeners are in charge of disposing the image they receive.

Added to template.

goderbauer · 2020-09-28T21:46:19Z

packages/flutter/lib/src/painting/image_stream.dart

+  /// already available, or if an error has been already reported, this object
+  /// will notify the listener synchronously.
+  ///
+  /// If the [ImageStreamCompleter] completes multiple images over its lifetime,


Template should cover both. I'll also update ImageStreamListener.onImage.

goderbauer · 2020-09-28T21:47:30Z

packages/flutter/lib/src/painting/image_stream.dart

+  /// {@macro flutter.painting.imageStream.addListener}
+  void addListener(ImageStreamListener listener) {
+    _checkDisposed();
+    _listeners.add(listener);


nit: rename _listeners to _activeListeners to avoid confusion in the code of what's included here?

goderbauer · 2020-09-28T21:53:56Z

packages/flutter/lib/src/widgets/basic.dart

+    );
    return RenderImage(
-      image: image,
+      image: image?.clone(),


Who disposes this?

The render object.

goderbauer · 2020-09-28T21:54:57Z

packages/flutter/lib/src/widgets/basic.dart

+    );
    renderObject
-      ..image = image
+      ..image = image?.clone()


I have also been confused by the fact that cloning and disposing are done by separate entities.

goderbauer · 2020-09-28T21:55:53Z

packages/flutter/lib/src/widgets/basic.dart


+  @override
+  void didUnmountRenderObject(RenderImage renderObject) {
+    renderObject.image = null;


This doesn't need to be disposed?

This causes the render object to dispose the image.

I've refactored RawImage into a public StatefulWidget and made the current impl private. The public impl now does not need to be special cased.

goderbauer · 2020-09-28T21:56:43Z

packages/flutter/lib/src/widgets/image.dart

    });
  }

+  void _disposeImage([ImageInfo? newInfo]) {


This is oddly named since it does not dispose the image from the info given to it. Maybe rename to replaceImage?

goderbauer · 2020-09-28T21:56:52Z

packages/flutter/lib/src/widgets/image.dart

+    // if (createPassiveListener) {
+    //   _imageStream!.completer?.addPassiveListener(null);
+    // }


haha whups :) Thought I needed this at one point but I do not.

Oh. I think I do need something like this. I'll fix it and add the relevant test.

The problem right now is that if the TickerMode goes off, we could inadvertently dispose of the image stream when we stop listening to it. We'll have to add a passive listener and remove it.

This reverts commit ea19b4e.

dnfield · 2020-09-29T03:28:14Z

My idea to refactor Rawimage doesn't quite work, because it still involves trying to make a Widget hold on to a stateful object. The basic problem is if we give RawImage a clone of the image, it will never know when to dispose of it. State.dispose isn't correct because it might create multiple states.

Hixie · 2020-09-30T00:31:31Z

packages/flutter/lib/src/painting/image_cache.dart

+    // Give any interested parties a chance to listen to the stream before we
+    // potentially dispose it.
+    // scheduleMicrotask(() {
+    SchedulerBinding.instance!.addPostFrameCallback((Duration timeStamp) {


addPostFrameCallback doesn't request a new frame. Are you guaranteed that this code is called only during a frame?

No, but we want to keep it alive until the next scheduled frame to give other clients time to subscribe.

The downside is that if you precache an image and then never ever schedule a frame again, you'll hold the memory. But I'm not clear on how that would happen - you shouldn't be precaching images if you're not going to schedule frames to draw them.

Hixie · 2020-09-30T00:36:23Z

packages/flutter/lib/src/painting/image_stream.dart

    : assert(image != null),
      assert(scale != null);

+  /// Creates an [ImageInfo] with a cloned [image].


i think this class should have a dispose, since it owns something that can be disposed. That would be more intuitive than having to clean out its contents.

The only reason I didn't do that is because we have some objects that eventually are just given the image out of an ImageInfo.

I think it's confusing either way, I'm just not sure which way is less confusing :)

Hixie · 2020-09-30T01:59:06Z

packages/flutter/lib/src/widgets/image.dart


    if (!widget.gaplessPlayback)
-      setState(() { _imageInfo = null; });
+      setState(() => _replaceImage(info: null));


avoid => in setState since it doesn't return anything

Hixie

I think this is the best we've come up with for this problem. I continue to hope that one day we can find an even better solution, but I've no idea what that could even be.

Hixie · 2020-10-01T16:45:18Z

(LGTM)

goderbauer

LGTM with some tiny nits.

goderbauer · 2020-10-01T16:49:26Z

packages/flutter/lib/src/painting/image_provider.dart

 ///   void _updateImage(ImageInfo imageInfo, bool synchronousCall) {
 ///     setState(() {
 ///       // Trigger a build whenever the image changes.
+///       _imageInfo?.image?.dispose();


Should you dispose the _imageInfo directly here instead of the image?

goderbauer · 2020-10-01T16:49:35Z

packages/flutter/lib/src/painting/image_provider.dart

 ///   @override
 ///   void dispose() {
 ///     _imageStream.removeListener(ImageStreamListener(_updateImage));
+///     _imageInfo?.image?.dispose();


goderbauer · 2020-10-01T16:51:44Z

packages/flutter/lib/src/widgets/image.dart

+  ///
+  /// If the listener from this state is the last listener on the stream, the
+  /// stream will be disposed. To keep the stream alive, set `keepStreamAlive`
+  /// to true, which will add a passive listener on the stream and is compatible


nit: we no longer have passive listeners.

goderbauer · 2020-10-01T16:55:43Z

packages/flutter/lib/src/painting/image_stream.dart

+  ///   // If the image reference is exactly the same, or its a clone of the
+  ///   // current reference, we can immediately dispose of it and avoid
+  ///   // recalculating anything.
+  ///   if (value == _imageInfo || value != null && _imageInfo != null && value.isCloneOf(_imageInfo)) {


Change this to what the actual code in rendering/image.dart is? I.e. don't dispose if value == _imageInfo!

goderbauer · 2020-10-01T16:56:50Z

packages/flutter/lib/src/painting/image_stream.dart

+  ///
+  /// See also:
+  ///
+  ///  * [Image.clone]


nit: add some context why to look there.

goderbauer · 2020-10-01T16:58:13Z

packages/flutter/lib/src/painting/image_stream.dart

+  /// Disposes of this object.
+  ///
+  /// Once this method has been called, the object should not be used anymore,
+  /// and no clones of it or the image it contains can be made.


I wonder if it is worthwhile to add asserts that a disposed imageinfo isn't used anymore (e.g. assert in the image and clone getter?)

goderbauer · 2020-10-01T16:59:40Z

packages/flutter/lib/src/painting/image_stream.dart

  @visibleForTesting
  bool get hasListeners => _listeners.isNotEmpty;

+  /// We should avoid disposing a completer if it has never had a listener, even


nit: "should avoid" sounds pretty weak. Are there situations where we dispose this even though somebody anted it alive?

This reverts commit a795469.

* Revert "docs for image disposal (#67066)" This reverts commit bcb2ac5. * Revert "Dispose of images after using them (#66688)" This reverts commit a795469.

liyuqian · 2020-10-02T18:24:17Z

Before the revert, we detected a big reduction in max memory usage. Look forward to the reland!

Dispose of images after using them

26fa02a

dnfield requested review from Hixie, goderbauer and jonahwilliams September 25, 2020 21:53

flutter-dashboard bot added a: tests "flutter test", flutter_test, or one of our tests framework flutter/packages/flutter repository. See also f: labels. labels Sep 25, 2020

googlebot added the cla: yes label Sep 25, 2020

dnfield added 2 commits September 25, 2020 14:58

Wrong import, missing asserts

be3b62c

more analysis

bcf72db

dnfield marked this pull request as draft September 25, 2020 22:29

dnfield added 2 commits September 28, 2020 11:44

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

7555775

Fix code sample

1c6803f

dnfield marked this pull request as ready for review September 28, 2020 18:57

fix null checks

4608a80

jonahwilliams reviewed Sep 28, 2020

View reviewed changes

jonah review

7d5b99e

jonahwilliams reviewed Sep 28, 2020

View reviewed changes

goderbauer reviewed Sep 28, 2020

View reviewed changes

dnfield added 5 commits September 28, 2020 15:58

Goderbauer review

7c94b56

skips for web

0d2db79

Refactor RawImage

ea19b4e

Revert "Refactor RawImage"

d6bcfa0

This reverts commit ea19b4e.

fix typo

db775af

Hixie reviewed Sep 30, 2020

View reviewed changes

unused imports

1bf11c4

Hixie reviewed Sep 30, 2020

View reviewed changes

dnfield added 9 commits September 29, 2020 20:09

two nits

49a810b

..

9fb35b1

ImageInfo.dispose

3dea1e3

update mock

0ac8d77

merge

4203fed

ci

cc59c70

Merge branch 'master' into image_dispose_better

634937f

missing end-tool

6404450

missing inner dispose calls

7d62a9a

Hixie approved these changes Oct 1, 2020

View reviewed changes

dnfield merged commit a795469 into flutter:master Oct 1, 2020

dnfield deleted the image_dispose_better branch October 1, 2020 16:48

goderbauer approved these changes Oct 1, 2020

View reviewed changes

This was referenced Oct 1, 2020

docs for image disposal #67066

Merged

The image cache can lose track of a live image that it probably shouldn't lose track of #67069

Open

removing Image widgets does not reduce Graphics memory allocation #56482

Closed

dnfield added a commit to dnfield/flutter that referenced this pull request Oct 1, 2020

Revert "Dispose of images after using them (flutter#66688)"

e85e7b6

This reverts commit a795469.

dnfield mentioned this pull request Oct 1, 2020

Revert dispose images when done #67100

Merged

dnfield added a commit that referenced this pull request Oct 1, 2020

Revert dispose images when done (#67100)

473358d

* Revert "docs for image disposal (#67066)" This reverts commit bcb2ac5. * Revert "Dispose of images after using them (#66688)" This reverts commit a795469.

dnfield mentioned this pull request Oct 2, 2020

Implement Image.clone for CanvasKit flutter/engine#21555

Merged

liyuqian added perf: memory Performance issues related to memory c: performance Relates to speed or footprint issues (see "perf:" labels) labels Oct 2, 2020

dnfield mentioned this pull request Oct 2, 2020

Add benchmark/test for drawing images across frames #67176

Merged

kinarobin mentioned this pull request Feb 23, 2021

Dispose image when cache clear #76600

Merged

8 tasks

Conversation

dnfield commented Sep 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Breaking Change

Uh oh!

dnfield commented Sep 25, 2020

Uh oh!

dnfield commented Sep 25, 2020

Uh oh!

dnfield commented Sep 25, 2020

Uh oh!

dnfield commented Sep 28, 2020

Uh oh!

dnfield commented Sep 28, 2020

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!

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 Sep 25, 2020 •

edited

Loading