Image dispose #64582

dnfield · 2020-08-25T20:06:48Z

Description

Some background:

In Flutter, Image objects are underpinned by SkImages, which are immutable refcounted objects. An SkImage cannot release its underlying image data until its refcount drops to zero.

We draw SkImages into SkPictures, so whatever picture we draw an image into also has to have its refcount go to zero to release the image memory.

SkPictures end up being held by Picture objects in Dart, which in turn are held by Layer objects. They are also held internally by the engine by PictureLayer objects, which do not have an exact Dart peer but are indirectly held by other Dart peer Layers (which hold on to EngineLayer objects eventually). Layers have a somewhat ambiguous ownership model - they are owned potentially by other layers or by RenderObjects. RenderObjects have no disposal method.

In an ideal world, we would have a proper lifecycle established for RenderObjects, Layers, EngineLayers, Pictures, and Images. Unfortunately, doing this will require a number of breaking changes. I've filed #64581 with most of the same text as this context to track.

In the mean time, we can improve the situation by calling dispose on an image when its listener count has dropped to zero. We can allow users to opt-out of this if they have a special ImageStream, ImageProvider, or ImageStreamCompleter implementation that caches images and reuses them in unexpected ways (the framework's own ImageCache does not directly hold references to ui.Image objects). We can also make sure we dispose previous frames for animated images.

We can then update the engine side of dispose on ui.Image so that it helps hint the Dart VM that a GC would be helpful the next time we notify idle. That PR is pending here:

Related Issues

Part of #56482

A failed attempt to do this in a more generic way is here: flutter/engine#19842

It failed because it GC'd too often, which caused excessive CPU utilization.

Tests

I added the following tests:

Tests that the various ImageStream implementers do the expected thing.

Update to an unrelated test for nullsafety differences between test/framework code.

Update an existing test to opt-in to keeping the image alive, since it does weird things with the ui.Image objects.

Checklist

Before you create this PR, confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

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

(TBD)

No, no existing tests failed, so this is not a breaking change.
Yes, this is a breaking change. If not, delete the remainder of this section.
- I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
- I got input from the developer relations team, specifically from: Replace with the names of who gave advice
- I wrote a migration guide: https://flutter.dev/go/breaking-changes-template Replace this with a link to a pull request that adds the migration guide to https://flutter.dev/docs/release/breaking-changes

jonahwilliams · 2020-08-25T20:09:47Z

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

+  /// If multiple [ImageStream]s may refer to the same image, this should be set
+  /// to true. Within the framework, this does not normally occur, but custom
+  /// [ImageProvider] or [ImageStream] implementations can behave differently.
+  final bool keepAlive;


does re-using the framework's "keepAlive" nomenclature here have some potential for confusion? i.e. if users think that keep alive in the framework might be necessary for perf/state, might naming this similarly lead them to believe this should also be true?

In my mind it's a similar trade-off.

The only users who should be dealing with this are ones who are writing their own image providers, streams, or caches. If you think it'd help to expand the documentation here I'd be happy to, or if there's a clearer name for this that would be less confusing I'm not stuck on it.

you could sort of invert the naming here - use it to describe what additional action to be taken. Instead of keeping it alive, you are eagerly disposing it?

Maybe autoDispose?

jonahwilliams

Do we have any sort of test scenarios that show this using less memory/GC-ing correctly?

dnfield · 2020-08-25T20:46:16Z

It won't use less memory until the related engine change lands. But the related engine change will need this work.

I'm trying to push up the latest commits to my engine branch right now but GitHub is acting up.

dnfield · 2020-08-25T21:05:29Z

Draft here: flutter/engine#20754

Have to figure out why this is segfaulting flutter_test for a test that should be unrelated.

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

jonahwilliams · 2020-08-26T18:04:58Z

packages/flutter/test/painting/mocks_for_image_cache.dart


 class TestImageInfo implements ImageInfo {
-  const TestImageInfo(this.value, { this.image, this.scale = 1.0, this.debugLabel, this.keepAlive = false });
+  const TestImageInfo(this.value, { this.image, this.scale = 1.0, this.debugLabel, this.keepAlive = true });


autoDispose?

Missed these, should be fixed now.

dnfield · 2020-08-26T18:06:58Z

I missed a case here where the ImageCache might be holding a reference to the completer, but all listeners are gone, and the ImageCache needs to keep the completer alive. Added a mutable property to the stream so the cache can tell it to stay alive in that case.

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

goderbauer · 2020-08-27T21:15:12Z

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

        });
      }
      _currentSizeBytes -= image.sizeBytes!;
+      image.completer.keepAlive = false;


Could somebody else have gained access to the completer and set keepAlive to true? We would be overwriting their request to keep the thing alive here. Does completer instead need to have a keepAlive ref count that gets decreased/increased and its only no longer kept alive when it reaches zero?

I was hoping to avoid adding a new kind of refcount parameter, but maybe that would be better here rather than relying on the listener list to be the refcount. That might resolve your other concern below as well.

I am not saying that a ref count is the right idea here. Just worried that just having a boolean leads to other problems...

goderbauer · 2020-08-27T21:16:58Z

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

      }
      _onLastListenerRemovedCallbacks.clear();
+      if (!keepAlive && _currentImage?.autoDispose == true) {
+        _currentImage!.image.dispose();


What if the last listener is removed and then another one is added?

It will only work if the imageCache still has a reference to the image.

I could add an assert about that.

Is that guaranteed at that in such a scenario the image would still be in the cache, though?

No, someone could have evicted it or just disabled the cache

goderbauer · 2020-08-27T21:21:59Z

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

  /// A string used for debugging purpopses to identify the source of this image.
  final String? debugLabel;

+  /// Whether or not the [image] should be disposed of when an associated


In a multi frame scenario this also appears to control whether the previous frame gets disposed when the next frame is available regardless of the listeners on the ImageStream, no?

goderbauer · 2020-08-27T21:30:56Z

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

      }
      _onLastListenerRemovedCallbacks.clear();
+      if (!keepAlive && _currentImage?.autoDispose == true) {
+        _currentImage!.image.dispose();


Also, the ownership model of the image seems a little wonky. Is ImageStreamCompleter really authorized to call dispose on it? Not clear to me...

In fact, the Image widget appears to remove its listener when the surrounding TickerMode is disabled (which presumably would dispose the image here), but the widget continues to hold on to the image object and continues to render it on screen. Most likely that will crash badly since the image is disposed? (If we don't have it, we should add a test for an Image widget inside a disabled TickerMode)

Hmm.

We could have the image widget tell the completer to keep the image alive even if listeners go to zero while the state is still around.

I agree this is not ideal.But we can't have the widget state own this, and we really need something to tell us when it's done with the image.

That sounds like a pretty cumbersome API and AFAIK it's not really a pattern we use elsewhere in the framework.

goderbauer · 2020-08-27T21:35:18Z

General comment: the ownership of the image object seems really wonky. It is not clear to me that ImageStreamCompleter can just dispose it when the listeners go away, see also the scenario I outlined in this comment: #64582 (comment)

Co-authored-by: Jonah Williams <[email protected]> Co-authored-by: Michael Goderbauer <[email protected]>

dnfield · 2020-08-27T22:58:01Z

Benchmark here: #64762

We should land the benchmark before landing this, assuming this can ever land :)

…_dispose

dnfield · 2020-08-29T23:39:55Z

I'm working on a revision of this that uses a disposable handle on the image. Will update in the next couple days.

dnfield · 2020-09-01T03:21:08Z

I've refactored this to create an obtainImageHandle API that callers have to dispose when they're done with the image.

I'm worried this might be breaking because ImageInfo isn't const anymore. CodeSearch tells me that we were the only one using it as const in a test so maybe it's ok.

dnfield · 2020-09-01T03:22:48Z

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

  /// [maximumSize] and [maximumSizeBytes].
  void _touch(Object key, _CachedImage image, TimelineTask? timelineTask) {
    assert(timelineTask != null);
+    assert(image is! _LiveImage);


If we put a LiveImage in the _cache, it won't get disposed of properly.

Maybe they shouldn't be subclasses. But then we duplicate some logic. I'm flexible on this one.

jonahwilliams · 2020-09-01T03:28:15Z

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

+
+  factory ImageHandle._create(ui.Image image) {
+    assert(image != null);
+    _handles[image] ??= ImageHandle._(image);


nit: avoid the !:

return _handles[image] ??= ImageHandle._(image);

jonahwilliams · 2020-09-01T03:28:25Z

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

+  /// be in a disposed state as well and will no longer be usable.
+  ///
+  /// The [debugLabel] may be used to identify the source of this image.
+  ImageInfo({


FYI: removing const is a breaking change.

Only if it breaks a contributed test or a test in google3.

yeah, that seems unlikely in this case - its not really a commonly used type

jonahwilliams · 2020-09-01T03:29:31Z

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

  }

+  /// Adds a listener callback that is called whenever a new concrete [ImageInfo]
+  /// object is available or an error is reported, if there are any listeners


the second part of the first sentence is confusing

Rephrasing to hopefully help.

jonahwilliams · 2020-09-01T03:33:20Z

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

+  ///
+  /// This always returns null if asserts are disabled, and is only meant for
+  /// use in debug mode or tests.
+  int? get debugRefCount {


generally we only add debug only code that is actually used by the framework (in debug mode), and not just for testing. Seems like a lot to pollute the API just for a unit test

We definitely have other cases where it's added only for tests (for example, debugAssertNoTransientCallbacks on SchedulerBinding). I can mark it @VisibleForTesting if you think that's better.

This could be helpful for application code, if someone is developing a new type of image provider and can't figure out why they still have references to it and who might be adding them. Alternatively, the tests that use it could instead just disable the image cache to avoid it adding more refs.

I think in this case it feels sort of me to me, because the API is otherwise so small. I admit though, this isn't really that important, but if you think you can test the ImageHandle without the getters, it would be better to YAGNI, right?

jonahwilliams · 2020-09-01T03:45:12Z

What I'm sort of missing at this point is and end-to-end of the caching procedures. I'm not sure where the best place for that to go would be, perhaps the imageCache itself?

dnfield · 2020-09-01T03:51:35Z

What do you have in mind for an end-to-end of caching procedures?

jonahwilliams · 2020-09-01T03:53:32Z

Something like "life of an image" would be helpful IMO. The audience would be someone who is trying to write a new Image widget, for example

dnfield · 2020-09-01T04:45:45Z

There's a bit of documentation around that in the [ImageProvider] class, and probably tests in there. Is that what you're looking for?

goderbauer · 2020-09-08T19:36:42Z

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

+///
+/// To get an ImageHandle, clients should call [ImageInfo.obtainImageHandle].
+///
+/// Clients of an [ImageStream] should call [dispose] on this object when they


This API still seems ... akward? In order to access ImageInfo.image I need to call a different method (ImageInfo.obtainImageHandle) first. Could that method just return the thing I want so it's actually natural to call it first?

I think to do that we'd have to make this a breaking change. This is an attempt to keep this "less-breaking" (it still requires that we make ImageInfo non-const).

I toyed with the idea of returning something that implements Image, which when you call dispose on it actually just decreases a refcount. The problem with that will be it isn't usable on the native side because it's not a real native backed object.

We could break this by @deprecated the image getter, and exposing the image off of ImageHandle, which you then dispose. Would that be better?

goderbauer · 2020-09-08T19:40:01Z

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

+/// Clients should _not_ call [ui.Image.dispose] directly, as it will prevent
+/// other clients from using the same image reference should they need to.
+class ImageHandle {
+  /// Do not use this, use [_create] instead to make sure that ImageHandles


This surprised me a little. When first thinking about this, I expected the ImageHandle object to have on clear owner (whoever obtained it from the ImageInfo) and when they are done with it, it gets disposed and thrown away... It is a little surprising that multiple "owners" can (and must) call dispose on this object.

This is basically just a ref-counter to a single ui.Image.

goderbauer · 2020-09-08T19:42:32Z

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

+    return _handles[image] ??= ImageHandle._(image);
+  }
+
+  static final Expando<ImageHandle> _handles = Expando<ImageHandle>();


Why an Expando? Instead of its magic, shouldn't we always know when something needs to get dropped out of it? (e.g. when the handle ref count gets to zero?)

Because we don't want to keep the ImageHandle or Images this stores alive.

The problem here is that if multiple ImageStreams are vending the same Image, e.g. because two different image providers are resolving to the same image. We want to make sure that a single Image always uses the same ImageHandle.

goderbauer · 2020-09-08T19:46:11Z

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

+  /// [ImageHandle.dispose] when the client no longer will use the image.
+  ui.Image get image {
+    assert(!_imageHandle.debugDisposed, 'Object has been disposed.');
+    assert(_imageHandle._refCount > 0, 'Call `obtainImageHandle` before accessing the image in an ImageInfo object.');


_refCount > 0 could mean that somebody else (not the current caller of image) has a valid handle, no? I guess, we can't really assert that the current caller has a valid handle...

(see my other comment above): That's why I wish we could have an API where you access the image from a valid handler...

Yes - it could be someone else keeping the refcount up, not the caller.

goderbauer · 2020-09-08T19:46:54Z

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

    }
  }

+  /// Adds a listener callback that is called only called if there are listeners


grammar: ... is called only called ...

goderbauer · 2020-09-08T19:48:31Z

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

    }
  }

+  /// Adds a listener callback that is called only called if there are listeners


I am not sure I understand what this paragraph is saying. When should I add a regular listener? When a passive one?

The image cache wants to listen without actually requesting more frames. Maybe there's a better name for this.

dnfield · 2020-09-09T21:46:29Z

I'm going to close this in favor of an alternative approach that will depend on another change in the engine.

Allows for reference counting of images before disposal. This will allow multiple callers to hold a reference to an image and dispose of their reference without disposing the underlying image until all handles have been disposed. This will be used by the framework to help resolve some of the kludge I was trying to introduce in flutter/flutter#64582

dnfield added 3 commits August 25, 2020 08:46

..

ca13a1a

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

57ae51a

Dispose images when listeners die, unless told to keep them alive

7b2dd79

dnfield requested review from goderbauer and jonahwilliams August 25, 2020 20:06

flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label Aug 25, 2020

googlebot added the cla: yes label Aug 25, 2020

jonahwilliams reviewed Aug 25, 2020

View reviewed changes

dnfield mentioned this pull request Aug 25, 2020

Use hint freed specifically for image disposal flutter/engine#20754

Merged

8 tasks

dnfield mentioned this pull request Aug 26, 2020

Fix tests that use bogus objects, update TestImageInfo interface #64599

Merged

dnfield added 3 commits August 25, 2020 23:27

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

d0dcbb5

keepAlive -> autoDispose

699efe0

make it work with the image cache

cbaaa6d

jonahwilliams reviewed Aug 26, 2020

View reviewed changes

packages/flutter/lib/src/painting/image_stream.dart Outdated Show resolved Hide resolved

jonahwilliams reviewed Aug 26, 2020

View reviewed changes

dnfield added 2 commits August 26, 2020 11:35

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

f2c2b02

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

0ef4394

goderbauer reviewed Aug 27, 2020

View reviewed changes

Apply suggestions from code review

b9e6df5

Co-authored-by: Jonah Williams <[email protected]> Co-authored-by: Michael Goderbauer <[email protected]>

dnfield mentioned this pull request Aug 27, 2020

Memory benchmark for showing large images in succession #64762

Merged

dnfield added 4 commits August 27, 2020 21:53

Merge branch 'image_dispose' of github.com:dnfield/flutter into image…

b985751

…_dispose

?

28b3de9

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

6bd8d37

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

7a19f1e

dnfield added 2 commits August 31, 2020 20:13

ImageHandle

d3aa5e9

merge

2df762b

dnfield requested review from goderbauer and jonahwilliams September 1, 2020 03:21

dnfield commented Sep 1, 2020

View reviewed changes

jonahwilliams reviewed Sep 1, 2020

View reviewed changes

review

a5af482

dnfield added 2 commits August 31, 2020 20:53

fix docs

e2cba26

another doc

b87d129

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

d1730ef

Piinks added the a: images Loading, displaying, rendering images label Sep 2, 2020

goderbauer reviewed Sep 8, 2020

View reviewed changes

dnfield mentioned this pull request Sep 8, 2020

removing Image widgets does not reduce Graphics memory allocation #56482

Closed

dnfield closed this Sep 9, 2020

dnfield mentioned this pull request Sep 9, 2020

Create an ImageHandle wrapper flutter/engine#21057

Merged

1 task

dnfield mentioned this pull request Sep 25, 2020

Dispose of images after using them #66688

Merged

1 task

github-actions bot locked as resolved and limited conversation to collaborators Sep 9, 2021

Image dispose #64582

Image dispose #64582

Uh oh!

Conversation

dnfield commented Aug 25, 2020

Description

Related Issues

Tests

Checklist

Breaking Change

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!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

dnfield commented Aug 25, 2020

Uh oh!

dnfield commented Aug 25, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnfield commented Aug 26, 2020

Uh oh!

Uh oh!

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 commented Aug 27, 2020

Uh oh!

dnfield commented Aug 27, 2020

Uh oh!

dnfield commented Aug 29, 2020

Uh oh!

dnfield commented Sep 1, 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!

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

Uh oh!

Choose a reason for hiding this comment

jonahwilliams Sep 1, 2020 •

edited

Loading