Live image cache by dnfield · Pull Request #50318 · flutter/flutter

dnfield · 2020-02-07T06:33:23Z

WIP - this should not land until #50316; design doc for this is at https://flutter.dev/go/widget-tree-image-cache, although the approach has changed a bit since writing the doc. /cc @goderbauer @ds84182 @liyuqian @gaaclarke @ignatz @alml

Description

Related Issues

fixes #48731
fixes #49456
fixes #45406

Tests

I added the following tests:

Assert that the cache can weakly hold an image even if it's otherwise too big to fit in the cache or the cache is disabled
Assert that the cache does not weakly hold an image that has no listeners.
Test that an ImageProvider can handle errors when trying to find its cache location
Test that an ImageProvider can find its cache location
Test that precacheImage weakly holds an image reference for a frame (but no longer)
Test that this is enough time for someone to grab the stream and hold the image in memory even if it is not in strongly in the cache.

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.

I don't believe this is a breaking change, but once this is cleaned up a bit I'll manually run it through google's internal test suites.

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: Replace with a link to your migration guide

dnfield · 2020-02-07T06:33:43Z

Argh. I meant to open this as a draft. Oh well.

dnfield · 2020-02-07T06:35:11Z

-    expect(imageStreamCompleter.listeners.length, 0);
+    // Make sure the second listener can be called re-entrantly.
+    listeners[0].onImage(null, null);
+    listeners[0].onImage(null, null);


This is meant to land in #50316

dnfield · 2020-02-07T06:57:11Z

+///
+/// To obtain an [ImageCacheLocation], use [FlutterImageCache.locationForKey] or
+/// [ImageProvider.findCacheLocation].
+class ImageCacheLocation {


/cc @jamesderlin who was looking for a way to know if an image was pending in the cache.

dnfield · 2020-02-07T08:22:27Z

+      final FlutterImageCache flutterImageCache = imageCache as FlutterImageCache;
+      expect(flutterImageCache.locationForKey(testImage).pending, true);
+      expect(flutterImageCache.locationForKey(testImage).weak, true);
+      flutterImageCache.clear();
+      expect(flutterImageCache.locationForKey(testImage).pending, false);
+      expect(flutterImageCache.locationForKey(testImage).weak, true);
+      flutterImageCache.clearWeakImages();
+      expect(flutterImageCache.locationForKey(testImage).pending, false);
+      expect(flutterImageCache.locationForKey(testImage).weak, false);


clear does remove the pending image, but this test no longer captures how to check if an image is pending or not.

This test could be independently rewritten without these changes to use the existing containsKey API, although the new API offers more robust ways to check if an image is pending.

Hixie · 2020-02-11T19:02:17Z

+  /// This method exists so that [FlutterImageCache] can add to its weak cache
+  /// once an [ImageStreamCompleter] has been created by this cache.
+  @protected
+  void _onImageStreamCompleterCreated(Object key, ImageStreamCompleter completer) {}


_handleImageStreamCompleterCreated per style guide

Removing this since we're just going to break the API

Hixie · 2020-02-11T19:03:18Z

+
+  /// Clears any weak references to images in this cache.
+  ///
+  /// This should not ordinarily be called by applications.


Rather than saying it shouldn't be called, say when it should be called.

Hixie · 2020-02-11T19:15:11Z

+/// This cache also exposes internal dimensions of the implementation, such as
+/// checking whether an image for a given key is pending, weak, or completed.
+/// See [ImageCacheLocation] for more details.
+class FlutterImageCache extends ImageCache {


in the api design review meeting we discussed just merging FlutterImageCache into ImageCache, and make the test-only features be methods called debugFoo... with their bodies in asserts.

I'm leaning towards making these non-debug/assert methods at this point - this could be valuable information at runtime (e.g. if an application wants to know how many live images are on the screen before decoding more). I'm also documenting [precacheImage] as relying on this behavior now.

Hixie · 2020-02-11T19:18:12Z

-      stream.removeListener(listener);
+      // Give callers at least one frame to subscribe to the image stream
+      // so that FlutterImageCache can weakly hold it without losing it.
+      SchedulerBinding.instance.scheduleFrameCallback((Duration timeStamp) {


should probably be schedulePostFrameCallback so that you don't actually schedule a frame

Done, and updated tests relevant to this to actually schedule a frame where needed.

Hixie · 2020-02-11T19:18:35Z

@@ -65,9 +65,11 @@ mixin PaintingBinding on BindingBase, ServicesBinding {



please avoid the word "weak" throughout

goderbauer · 2020-02-13T17:47:15Z

 /// the network if they are referenced in the [putIfAbsent] method), but the raw
 /// bits are kept in memory for as long as the application is using them.
 ///
+/// The cache also holds a list of "live" references. An image is considered


nit: the paragraph above says that active images may also get removed from the cache, this paragraph talks about live images. Maybe specify in the above paragraph when "active" images can get evicted (e.g. when they no longer are "live").

Also: It's probably not super clear what the difference between "actively in use" and live is...

I'm removing the above paragraph - thanks for catching that.

goderbauer · 2020-02-13T17:49:19Z

      return image.completer;
    }
+
+    result = _liveImages[key];


Shouldn't these go back into the regular cache (if they fit) if they have been used again?

Ah, yes. I'll fix this.

goderbauer · 2020-02-13T17:50:07Z

 class ImageCache {
  final Map<Object, _PendingImage> _pendingImages = <Object, _PendingImage>{};
  final Map<Object, _CachedImage> _cache = <Object, _CachedImage>{};
+  final Map<Object, ImageStreamCompleter> _liveImages = <Object, ImageStreamCompleter>{};


For future readers: Maybe add a comment here what "live" means?

goderbauer · 2020-02-13T17:51:38Z

  }

+  /// The [ImageCacheLocation] information for the given `key`.
+  ImageCacheLocation locationForKey(Object key) => ImageCacheLocation._(


nit: per style guide, use => only if everything fits on one line.

Done. I had this on one line, realized it was too long, and then forgot to change the fat arrow :)

goderbauer · 2020-02-13T17:51:58Z

    return _pendingImages[key] != null || _cache[key] != null;
  }

+  /// The number of live images being held by the [ImageCache]. Compare with


nit: first paragraph should be one sentence summary.

Same below.

Split these

goderbauer · 2020-02-13T18:05:14Z

+    // hence the need for the `didError` flag.
+    final Zone dangerZone = Zone.current.fork(
+      specification: ZoneSpecification(
+        handleUncaughtError: (Zone zone, ZoneDelegate delegate, Zone parent, Object error, StackTrace stackTrace) {


/cc @jonahwilliams

goderbauer · 2020-02-13T18:06:02Z

+
+  final List<VoidCallback> _onLastListenerRemovedCallbacks = <VoidCallback>[];
+
+  /// A adds a callback to call when [removeListener] results in an empty


nit: "A adds a..." ??

goderbauer · 2020-02-13T18:06:54Z

+  /// A adds a callback to call when [removeListener] results in an empty
+  /// list of listeners.
+  ///
+  /// This callback will never fire if [removeListener] is never called.


Feels odd that there's no way to remove a onlastlistenerremoved listener.

I don't have a use case for it. I also suspect that it would be better to track these by ID if we ever want to do that, to avoid the confusion about what happens when someone removes a duplicate listener. WDYT?

goderbauer · 2020-02-13T18:08:14Z

+/// as long as both images share the same key, and the image is held by the
+/// cache.
+///
+/// The cache may refuse to hold the image it is disabled, the image is too


Is this sentence missing an "if" or something?

goderbauer · 2020-02-13T18:08:43Z

+/// as long as both images share the same key, and the image is held by the
+/// cache.
+///
+/// The cache may refuse to hold the image it is disabled, the image is too


Also: there are two toos (one at the end of this line and one in the next line)

dnfield · 2020-02-14T01:13:09Z

I've merged in the image tracing tests. This should be fully ready for review and landing on approval.

dnfield · 2020-02-14T18:34:31Z

(Fixed some bugs in my last push - I had forgotten to delete some code and had a bad assert in there)

goderbauer · 2020-02-14T18:31:27Z


+  /// The number of live images being held by the [ImageCache].
+  ///
+  /// Compare with [ImageCache.currentSize] for completed held images.


Not sure what "completed held images" is?

Missed a text replacement. done.

goderbauer · 2020-02-14T18:32:25Z

+///
+/// A [pending] image is one that has not completed yet. It may also be tracked
+/// as [live] because something is listening to it.
+///


nit: remove one blank line.

goderbauer · 2020-02-14T18:37:03Z

+    };
+
+
+    // If an error is added to a synchronous completer before a listener has been


Wait, isn't the other occurence also in this file? Could this be just be refactored into a private method? I don't think there's any danger of anybody accessing a private method from here?

goderbauer · 2020-02-14T18:48:05Z

+      return;
+    }
+    _liveImages[key] = image;
+    image.completer.addOnLastListenerRemovedCallback(() {


What if nobody ever listens to the stream (and hence this callback never fires)? It appears that we'd never remove the image from the live cache?

dnfield · 2020-02-14T18:55:43Z

Ahh I got mixed up about the dangerzone stuff - I thought that one was on ImageCache. I'll refactor.

goderbauer

LGTM

dnfield · 2020-02-15T00:29:02Z

Quick update on this:

When I opened the PR, it was a breaking change.

During the course of work on this PR, the tests it broke no longer were breaking (in fact, they disappeared entirely!).

On landing it, it no longer breaks our compatibility guidelines (https://flutter.dev/docs/resources/compatibility). I'll still send an email to Flutter-announce explaining this.

dnfield · 2020-02-15T00:40:09Z

And if anyone would really like to see a migration guide for this, I can certainly write one :)

This reverts commit 1602be6.

This reverts commit 2f09d60.

* Revert "Revert "Live image cache (#50318)" (#51131)" This reverts commit 2f09d60. * Fix eviction of a pending image

lifelikejuly · 2020-05-11T08:09:47Z

Please I want to know version1.17.0 ImageCache add liveImage how to useful。

dnfield · 2020-05-11T13:41:40Z

It is automatic

lifelikejuly · 2020-05-11T14:04:12Z

It is automatic

Sorry, actually I mean want to ask what it does

dnfield · 2020-05-11T14:31:33Z

The design doc and linked PRs have more details, but in a nutshell this makes sure that once you resolve an image, it continues to be reachable as long as its listener count has not dropped to zero, even if it did not otherwise fit in the image cache. This is important especially for large images or cache configurations that do not allow for many images.

dnfield added 5 commits February 6, 2020 19:45

temp

c2adcff

Make onImage re-entrant safe

31a04ec

merge

da98159

Docs, tests

4580ec5

better test

4c5ae7d

fluttergithubbot added framework flutter/packages/flutter repository. See also f: labels. c: contributor-productivity Team-specific productivity, code health, technical debt. labels Feb 7, 2020

googlebot added the cla: yes label Feb 7, 2020

dnfield added the waiting for tree to go green label Feb 7, 2020

bad import

f9bf360

dnfield commented Feb 7, 2020

View reviewed changes

dnfield added 2 commits February 6, 2020 22:35

unintended pubspec change

cda0d6b

docs and comparisons

1dba7d2

dnfield commented Feb 7, 2020

View reviewed changes

dnfield added 2 commits February 6, 2020 23:42

merge

02aae6e

comment

45f9770

dnfield added work in progress; do not review and removed waiting for tree to go green labels Feb 7, 2020

update test

257ef91

dnfield force-pushed the weak_image_cache branch from d953dda to 257ef91 Compare February 7, 2020 08:20

dnfield commented Feb 7, 2020

View reviewed changes

This was referenced Feb 7, 2020

precacheImage should let you preserve the image it loads even if it doesn't fit in the cache #48731

Closed

Image cache eviction policy is too aggressive #45406

Closed

dnfield added 3 commits February 7, 2020 12:24

merge

6e7b93c

merge

d619811

merge

3cd8ee7

Hixie reviewed Feb 11, 2020

View reviewed changes

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

9b65965

dnfield changed the title ~~Weak image cache~~ Live image cache Feb 11, 2020

goderbauer reviewed Feb 13, 2020

View reviewed changes

dnfield added 4 commits February 13, 2020 10:39

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

b64c1f2

doc fixes, touch, edge cases for live images and sync calls

1cb06c4

status

f565da3

merge

827911e

test

d60d64e

dnfield force-pushed the weak_image_cache branch from 611b5a8 to d60d64e Compare February 14, 2020 17:13

goderbauer reviewed Feb 14, 2020

View reviewed changes

dnfield added 2 commits February 14, 2020 12:35

factor out common code

a0bd901

tests, listen even if disabled

01e4b25

goderbauer approved these changes Feb 14, 2020

View reviewed changes

dnfield merged commit 1602be6 into flutter:master Feb 14, 2020

dnfield deleted the weak_image_cache branch February 14, 2020 23:03

dnfield added the a: images Loading, displaying, rendering images label Feb 18, 2020

dnfield added a commit that referenced this pull request Feb 20, 2020

Revert "Live image cache (#50318)"

d506171

This reverts commit 1602be6.

dnfield mentioned this pull request Feb 20, 2020

Revert "Live image cache" #51131

Merged

dnfield added a commit that referenced this pull request Feb 20, 2020

Revert "Live image cache (#50318)" (#51131)

2f09d60

This reverts commit 1602be6.

dnfield added a commit to dnfield/flutter that referenced this pull request Feb 22, 2020

Revert "Revert "Live image cache (flutter#50318)" (flutter#51131)"

542c547

This reverts commit 2f09d60.

dnfield mentioned this pull request Feb 22, 2020

Live image cache #51249

Merged

dnfield added a commit that referenced this pull request Feb 24, 2020

Live image cache (#51249)

e2dcdb6

* Revert "Revert "Live image cache (#50318)" (#51131)" This reverts commit 2f09d60. * Fix eviction of a pending image

github-actions Bot locked as resolved and limited conversation to collaborators Jul 31, 2021

		@@ -65,9 +65,11 @@ mixin PaintingBinding on BindingBase, ServicesBinding {


		final List<VoidCallback> _onLastListenerRemovedCallbacks = <VoidCallback>[];

		/// A adds a callback to call when [removeListener] results in an empty

		};


		// If an error is added to a synchronous completer before a listener has been

Conversation

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

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnfield commented Feb 14, 2020

Uh oh!

dnfield commented Feb 14, 2020

dnfield commented Feb 7, 2020 •

edited

Loading