Make images contribute to Picture allocation size, more aggressively release references when dispose is called #18706

dnfield · 2020-06-01T17:34:03Z

SkImage references get held by both our Image and Picture objects. The GC has no idea about this, and so sometimes pictures look very small when they're actually holding a reference to a large chunk of memory. This patch helps make sure that the GC can more adequately catch the real size impact of Picture objects, and combined with an upstream patch in Dart allows for much more aggressive collection of unused image related memory.

Unfortunately, this may give an incorrect view of memory to devtools - /cc @liyuqian @terrylucas @devoncarew. The fact that we're reporting image memory size in two places does not necessarily mean we've allocated that much memory in the backend (e.g. Skia might have some kind of CoW feature at work, and the Image is only referenced in a single Picture).

Fixes flutter/flutter#57746
Part of flutter/flutter#56482 (needs https://dart-review.googlesource.com/c/sdk/+/149521 as well for that issue)
Fixes flutter/flutter#28912

/cc @ds84182

dnfield · 2020-06-01T17:36:37Z

lib/ui/painting/canvas.h

@@ -106,12 +106,12 @@ class Canvas : public RefCountedDartWrappable<Canvas> {
  void drawPath(const CanvasPath* path,
                const Paint& paint,
                const PaintData& paint_data);
-  void drawImage(const CanvasImage* image,
+  void drawImage(CanvasImage* image,


The reason to drop const is GetAllocationSize is not const. Filed flutter/flutter#58422 to investigate making it const and adding the qualifiers back here.

Looks like you just have to add the const qualifier to GetAllocationSize. I checked out the implementations and didn't see any mutations.

Can you just address this issue in this patch? Tonic is in this repo now so it should be straightforward. You call.

Opened #18713 so that doesn't get reverted if this does. I'll merge that one into this.

gaaclarke · 2020-06-01T18:48:08Z

testing/dart/canvas_test.dart

+    canvas.drawImageRect(image, rect, rect, Paint());
+    canvas.drawImageNine(image, rect, rect, Paint());
+    final Picture picture = recorder.endRecording();
+    expect(picture.approximateBytesUsed, 161235);


I'd break down the calculation of these numbers.

That'll be more possible if we expose this on Canvas, which as I think about it, we should probably do. @chinmaygarde does that SGTY? While the Canvas is still alive, it holds a reference to the SkImage in this case.

We don't really expose as much on the Dart side here either - for example, we don't expose the image allocation size

(and if there's something I'm missing here that you think would just make the test better I'm open to suggestions)

I meant like

int expectedBytes = /*foo*/ 123 + /*bar*/ 567 + (4 * /*baz*/ 89); expect (picture.approximateBytesUsed, expectedBytes);

Ahh ok, I can do that :)

gaaclarke · 2020-06-01T18:49:58Z

lib/ui/painting/canvas.h

@@ -106,12 +106,12 @@ class Canvas : public RefCountedDartWrappable<Canvas> {
  void drawPath(const CanvasPath* path,
                const Paint& paint,
                const PaintData& paint_data);
-  void drawImage(const CanvasImage* image,
+  void drawImage(CanvasImage* image,


Looks like you just have to add the const qualifier to GetAllocationSize. I checked out the implementations and didn't see any mutations.

gaaclarke · 2020-06-01T18:50:34Z

lib/ui/painting/picture.h

@@ -43,10 +45,14 @@ class Picture : public RefCountedDartWrappable<Picture> {
                                      uint32_t height,
                                      Dart_Handle raw_image_callback);

+  size_t ImageAllocationSize() { return image_allocation_size_; }


const method

gaaclarke · 2020-06-01T18:51:55Z

lib/ui/painting/canvas.cc

@@ -86,7 +86,7 @@ fml::RefPtr<Canvas> Canvas::Create(PictureRecorder* recorder,
  return canvas;
 }

-Canvas::Canvas(SkCanvas* canvas) : canvas_(canvas) {}
+Canvas::Canvas(SkCanvas* canvas) : canvas_(canvas), image_allocation_size_(0) {}


fyi some people prefer initialization like this in the header since it's less likely to cause problems in the future if someone adds a constructor. I'm fine as long as there is an error if it isn't initialized.

gaaclarke · 2020-06-01T18:53:29Z

lib/ui/painting/canvas.cc

@@ -293,10 +293,11 @@ void Canvas::drawImage(const CanvasImage* image,
  if (!image)
    Dart_ThrowException(
        ToDart("Canvas.drawImage called with non-genuine Image."));
+  image_allocation_size_ += image->GetAllocationSize();
  canvas_->drawImage(image->image(), x, y, paint.paint());


Is this accurate? I would have expected the canvas to rasterize the drawn image at some point so it doesn't actually hold on to the full image.

It's going to hold a reference to the SkImage as long as its alive. The memory is not necessarily duplicated (e.g. we don't necessarily have 2x the pixel allocation for the extra reference), but we need to do a better job of letting the GC know how expensive the object is, otherwise it will think it can skip a GC pass when it really needs one.

It might help to clarify the semantics of "allocation size". The allocation size is not the size of the allocation that will be collected when the owning object is collected. Instead, it is the size of the allocation that is guaranteed to stay alive pending the collection of the owning object.

As in a doc comment in dart_wrappable.h?

Sure. I don't know of a better place.

chinmaygarde · 2020-06-01T18:48:38Z

lib/ui/painting/canvas.cc

@@ -86,7 +86,7 @@ fml::RefPtr<Canvas> Canvas::Create(PictureRecorder* recorder,
  return canvas;
 }

-Canvas::Canvas(SkCanvas* canvas) : canvas_(canvas) {}
+Canvas::Canvas(SkCanvas* canvas) : canvas_(canvas), image_allocation_size_(0) {}


Can you just default this in the header please. This TU is old enough that that this wasn't possible when it was first written.

chinmaygarde · 2020-06-01T18:52:49Z

lib/ui/painting/canvas.h

@@ -106,12 +106,12 @@ class Canvas : public RefCountedDartWrappable<Canvas> {
  void drawPath(const CanvasPath* path,
                const Paint& paint,
                const PaintData& paint_data);
-  void drawImage(const CanvasImage* image,
+  void drawImage(CanvasImage* image,


Can you just address this issue in this patch? Tonic is in this repo now so it should be straightforward. You call.

chinmaygarde · 2020-06-01T18:58:08Z

lib/ui/painting/canvas.cc

@@ -348,6 +351,7 @@ void Canvas::drawPicture(Picture* picture) {
  if (!picture)
    Dart_ThrowException(
        ToDart("Canvas.drawPicture called with non-genuine Picture."));
+  image_allocation_size_ += picture->GetAllocationSize();


You missed a few:

drawAtlas references an image.

drawVertices and drawPoints has vertex buffers whose sizes can be appreciable (IIRC stuff like flare uses it extensively).

drawPath can have custom paths that may be large. There are path iterator APIs that you can use to walk an count the verbs and points. You should probably memoize the size here too.

All arguments that can take a paint may have a shader set on them. These shaders can reference images. In the same vein, shaders in dart:ui should also reference the correct size.

I understand if you don't want to audit the existing codebase and track everything in this one patch. Let's file a bug to follow up on to perform correct book-keeping on the resources referenced by dart:ui call.

I'm including drawAtlas in this patch. Filed flutter/flutter#58438 for the rest.

Filed flutter/flutter#58440 specifically for ImageShader, since that's a little different.

Sounds good. Thanks.

chinmaygarde · 2020-06-01T19:05:44Z

lib/ui/painting/canvas.h

@@ -170,13 +170,16 @@ class Canvas : public RefCountedDartWrappable<Canvas> {

  static void RegisterNatives(tonic::DartLibraryNatives* natives);

+  size_t ImageAllocationSize() { return image_allocation_size_; }


Nit on naming convention (though this is a WebKit originating TU so the style is all over the place). image_allocation_size if inline and GetImageAllocationSize if OOL.

Swtiching to image_allocation_size. I think this is right?

chinmaygarde

Looks great! Thanks.

…ssively release references when dispose is called (flutter/engine#18706)

skquo · 2020-08-17T08:36:07Z

PictureRecorder crash 63574

dnfield added 5 commits May 27, 2020 14:51

start

1150bc2

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

2e82317

Fix issue with initialization of image size

5398e9b

test

fd87388

stray newline

3d7a601

dnfield requested review from chinmaygarde, jason-simmons and gaaclarke June 1, 2020 17:34

auto-assign bot requested a review from cbracken June 1, 2020 17:34

googlebot added the cla: yes label Jun 1, 2020

dnfield mentioned this pull request Jun 1, 2020

Consider making GetAllocationSize const in the engine flutter/flutter#58422

Closed

dnfield commented Jun 1, 2020

View reviewed changes

gaaclarke reviewed Jun 1, 2020

View reviewed changes

chinmaygarde reviewed Jun 1, 2020

View reviewed changes

Make GetAllocationSize const

e445fd7

dnfield mentioned this pull request Jun 1, 2020

Make GetAllocationSize const #18713

Merged

Merge branch 'const_alloc_size' into image_gc

b2d1765

dnfield mentioned this pull request Jun 1, 2020

Make ImageShaders update the allocation information when an image is set flutter/flutter#58440

Open

dnfield added 4 commits June 1, 2020 14:20

review

b9b06ba

explain test

15bf5da

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

017c9d5

Merge branch 'const_alloc_size' into image_gc

4416c3d

liyuqian mentioned this pull request Jun 1, 2020

Expose graphics memory usage information from engine flutter/flutter#56486

Closed

2 tasks

make test more lenient

7ccaede

dnfield mentioned this pull request Jun 1, 2020

removing Image widgets does not reduce Graphics memory allocation flutter/flutter#56482

Closed

chinmaygarde approved these changes Jun 1, 2020

View reviewed changes

dnfield added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Jun 2, 2020

dnfield merged commit f46dde1 into flutter:master Jun 2, 2020

dnfield deleted the image_gc branch June 2, 2020 22:12

dnfield mentioned this pull request Jun 3, 2020

Implement GetAllocationSize for Vertices #18756

Merged

1 task

engine-flutter-autoroll mentioned this pull request Jun 3, 2020

Roll Engine from 859d892f1fca to 895ef3375ec6 (7 revisions) flutter/flutter#58632

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 3, 2020

f46dde1 Make images contribute to Picture allocation size, more aggre…

4d08dfa

…ssively release references when dispose is called (flutter/engine#18706)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 4, 2020

f46dde1 Make images contribute to Picture allocation size, more aggre…

da8bab2

…ssively release references when dispose is called (flutter/engine#18706)

This was referenced Jun 4, 2020

Roll Engine from 859d892f1fca to 2608f2ee9f54 (9 revisions) flutter/flutter#58647

Merged

Roll Engine from 859d892f1fca to a27336bcbf34 (16 revisions) flutter/flutter#58695

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 4, 2020

f46dde1 Make images contribute to Picture allocation size, more aggre…

fbf2408

…ssively release references when dispose is called (flutter/engine#18706)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 4, 2020

f46dde1 Make images contribute to Picture allocation size, more aggre…

1499e2b

…ssively release references when dispose is called (flutter/engine#18706)

engine-flutter-autoroll mentioned this pull request Jun 4, 2020

Roll Engine from 859d892f1fca to d3427ddb8546 (17 revisions) flutter/flutter#58698

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 4, 2020

f46dde1 Make images contribute to Picture allocation size, more aggre…

c9f1ea3

…ssively release references when dispose is called (flutter/engine#18706)

engine-flutter-autoroll mentioned this pull request Jun 4, 2020

Roll Engine from 859d892f1fca to d3427ddb8546 (17 revisions) flutter/flutter#58710

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 4, 2020

f46dde1 Make images contribute to Picture allocation size, more aggre…

a0219a9

…ssively release references when dispose is called (flutter/engine#18706)

engine-flutter-autoroll mentioned this pull request Jun 4, 2020

Roll Engine from 859d892f1fca to a62f724668d5 (20 revisions) flutter/flutter#58721

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 5, 2020

f46dde1 Make images contribute to Picture allocation size, more aggre…

12715d3

…ssively release references when dispose is called (flutter/engine#18706)

engine-flutter-autoroll mentioned this pull request Jun 5, 2020

Roll Engine from 859d892f1fca to 8cc760065b4d (29 revisions) flutter/flutter#58751

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make images contribute to Picture allocation size, more aggressively release references when dispose is called #18706

Make images contribute to Picture allocation size, more aggressively release references when dispose is called #18706

dnfield commented Jun 1, 2020

dnfield Jun 1, 2020

gaaclarke Jun 1, 2020

chinmaygarde Jun 1, 2020

dnfield Jun 1, 2020

gaaclarke Jun 1, 2020

dnfield Jun 1, 2020

dnfield Jun 1, 2020

dnfield Jun 1, 2020

gaaclarke Jun 1, 2020

dnfield Jun 1, 2020

gaaclarke Jun 1, 2020

gaaclarke Jun 1, 2020

dnfield Jun 1, 2020

gaaclarke Jun 1, 2020

dnfield Jun 1, 2020

gaaclarke Jun 1, 2020

dnfield Jun 1, 2020

chinmaygarde Jun 1, 2020

dnfield Jun 1, 2020

chinmaygarde Jun 2, 2020

chinmaygarde Jun 1, 2020

dnfield Jun 1, 2020

chinmaygarde Jun 1, 2020

chinmaygarde Jun 1, 2020

dnfield Jun 1, 2020

dnfield Jun 1, 2020

chinmaygarde Jun 1, 2020

chinmaygarde Jun 1, 2020

dnfield Jun 1, 2020

chinmaygarde Jun 1, 2020

chinmaygarde left a comment

skquo commented Aug 17, 2020

		@@ -170,13 +170,16 @@ class Canvas : public RefCountedDartWrappable<Canvas> {

		static void RegisterNatives(tonic::DartLibraryNatives* natives);

		size_t ImageAllocationSize() { return image_allocation_size_; }

Make images contribute to Picture allocation size, more aggressively release references when dispose is called #18706

Make images contribute to Picture allocation size, more aggressively release references when dispose is called #18706

Conversation

dnfield commented Jun 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chinmaygarde left a comment

Choose a reason for hiding this comment

skquo commented Aug 17, 2020