Bug 53281 – To determine image properties, CG allocates memory which isn't included in CachedImage's decoded size

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Ian Henderson]

After receiving new image data, BitmapImage must ask CG whether the image encoded by the data has a well-defined size (and, if it is well-defined, what that size is).  To answer this question, CG decodes some part of the image.  The decoded data remains associated with the CGImageSource, but it is not accounted for in the associated CachedImage's decoded size.

If WebCore never paints the image, there's no need to keep the decoded data around.  We should clear the image source once we know what size the image is.

[Ian Henderson]

Created attachment 80405 [details]
1000-images test case

Attaching 1000-images test case which exhibits this issue.

[Ian Henderson]

Created attachment 80406 [details]
proposed patch

Proposed patch attached.  With the patch, loading the attached 1000-images uses about 10 megabytes less memory.

1000-images in ToT r76862 (5 runs, vmmap -resident):
1-old.vmmap:Writable regions: Total=2.1G written=51.4M(2%) resident=75.5M(3%) swapped_out=0K(0%) unallocated=2.1G(97%)
2-old.vmmap:Writable regions: Total=2.2G written=52.1M(2%) resident=76.2M(3%) swapped_out=0K(0%) unallocated=2.1G(97%)
3-old.vmmap:Writable regions: Total=2.2G written=50.5M(2%) resident=74.6M(3%) swapped_out=0K(0%) unallocated=2.1G(97%)
4-old.vmmap:Writable regions: Total=2.1G written=49.7M(2%) resident=73.8M(3%) swapped_out=0K(0%) unallocated=2.1G(97%)
5-old.vmmap:Writable regions: Total=2.1G written=52.2M(2%) resident=76.3M(3%) swapped_out=0K(0%) unallocated=2.1G(97%)

1000-images with 53281-1.patch applied (5 runs, vmmap -resident):
1-new.vmmap:Writable regions: Total=2.1G written=40.8M(2%) resident=64.9M(3%) swapped_out=0K(0%) unallocated=2.1G(97%)
2-new.vmmap:Writable regions: Total=2.1G written=39.4M(2%) resident=63.5M(3%) swapped_out=0K(0%) unallocated=2.1G(97%)
3-new.vmmap:Writable regions: Total=2.1G written=39.2M(2%) resident=63.3M(3%) swapped_out=0K(0%) unallocated=2.1G(97%)
4-new.vmmap:Writable regions: Total=2.1G written=40.1M(2%) resident=64.3M(3%) swapped_out=0K(0%) unallocated=2.1G(97%)
5-new.vmmap:Writable regions: Total=2.1G written=41.7M(2%) resident=65.8M(3%) swapped_out=0K(0%) unallocated=2.1G(97%)

[Andreas Kling]

This doesn't cause the image to be (at least partially) decoded twice in the case where it's painted?

[Eric Seidel (no email)]

Comment on attachment 80406 [details]
proposed patch

This seems like the wrong place to put this.  Should call some separate function which only does something on CG.

[Eric Seidel (no email)]

Hyatt knows about the CG image decoders.  I'm not sure who else does.

[Ian Henderson]

(In reply to comment #3)
> This doesn't cause the image to be (at least partially) decoded twice in the case where it's painted?

You're right -- at the time the size becomes available, the image never has any decoded frames, even if it needs to be decoded later to draw the page.  The code I added to catch this case doesn't actually work.

Would it be better to include the extra allocations in CachedImage's decodedSize?  Then the cache would clean up the data during a purge.

[David Kilzer (:ddkilzer)]

<rdar://problem/8684763>

[Ian Henderson]

Created attachment 80705 [details]
track extra CG allocations as part of an image's decoded size

Here's a revised patch that adds accounting for the extra CG allocations in BitmapImage.  It calls a method on ImageSource to determine how much the source will allocate to determine properties of the image—non-CG image sources return 0 to preserve the original behavior.

[Darin Adler]

Comment on attachment 80705 [details]
track extra CG allocations as part of an image's decoded size

View in context: https://bugs.webkit.org/attachment.cgi?id=80705&action=review

Looks good, on the right track.

But there’s enough here that needs some additional thought and revision that I am going to say review-.

> Source/WebCore/ChangeLog:5
> +        Clean up CG allocations made while determining image size

I don’t understand the title of this bug. This patch doesn’t seem to “clean up allocations”.

> Source/WebCore/platform/graphics/BitmapImage.cpp:88
> -    destroyMetadataAndNotify(framesCleared);
> +    destroyMetadataAndNotify(framesCleared, true);

We frown on this type of boolean argument in WebKit code. Seeing “true” here doesn’t tell you what’s going on. Generally we either avoid these kinds of arguments altogether or use enums so you can read the constant at the call site.

> Source/WebCore/platform/graphics/BitmapImage.cpp:103
> +void BitmapImage::destroyMetadataAndNotify(int framesCleared, bool propertiesDataCleared)

The meaning of the boolean “properties data cleared” is not clear to me. Who cleared the properties data? What exactly does that entail. I guess the existing “framesCleared” argument is already confusing in a similar way.

> Source/WebCore/platform/graphics/BitmapImage.cpp:151
> +void BitmapImage::sourceDecodedProperties() const

I don’t understand this function name. It seems to tell the image to “source” the “decoded properties”, but I’m not sure what either of those terms means.

Maybe it’s a function for the “source” to call that says that it has “decoded properties”. Probably best to name it “sourceDidDecodeProperties” or “didDecodeProperties” in that case.

> Source/WebCore/platform/graphics/BitmapImage.cpp:159
> +    int deltaBytes = updatedSize;
> +    deltaBytes -= m_decodedPropertiesSize;

It would be a little more conventional to just write:

    int deltaBytes = updatedSize - m_decodedPropertiesSize;

Subtracting two size_t and putting the result into an int could result in overflow. What guarantees the difference in size will fit in an int?

> Source/WebCore/platform/graphics/cg/ImageSourceCG.cpp:227
> +    // Measured by tracing malloc/calloc calls.

This definitely can’t be correct for both 32-bit and 64-bit systems, nor for multiple versions of CG. So the comment should explain that it’s OK to have this value that’s likely to be inaccurate. It’s good that your comment explains you got the number by tracing on one particular system and version, but it also needs to explain that it’s OK for to have an estimate here that might be wrong.

[Ian Henderson]

Thanks for the review, Darin.  I've updated the title of the bug to describe the underlying issue rather than any particular fix.

[Ian Henderson]

(In reply to comment #9)
> (From update of attachment 80705 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80705&action=review
> 
> > Source/WebCore/platform/graphics/BitmapImage.cpp:159
> > +    int deltaBytes = updatedSize;
> > +    deltaBytes -= m_decodedPropertiesSize;
> 
> It would be a little more conventional to just write:
> 
>     int deltaBytes = updatedSize - m_decodedPropertiesSize;
> 
> Subtracting two size_t and putting the result into an int could result in overflow. What guarantees the difference in size will fit in an int?

Is there a standard way in WebCore to get a signed difference of unsigned values?  Should I add an ASSERT that checks for overflow?

[Ian Henderson]

Created attachment 80810 [details]
patch v3

Updated patch to address review comments.

[Darin Adler]

(In reply to comment #11)
> Is there a standard way in WebCore to get a signed difference of unsigned values?  Should I add an ASSERT that checks for overflow?

Generally with unsigned values check which number is greater before doing any subtraction. Not sure if that’s helpful in this case. Since you already posted a new patch I’m assuming you did something. I’ll see when I get a chance to review.

[Darin Adler]

Comment on attachment 80810 [details]
patch v3

View in context: https://bugs.webkit.org/attachment.cgi?id=80810&action=review

> Source/WebCore/platform/graphics/BitmapImage.cpp:163
> +#ifndef NDEBUG
> +    bool overflow = updatedSize > m_decodedPropertiesSize && deltaBytes < 0;
> +    bool underflow = updatedSize < m_decodedPropertiesSize && deltaBytes > 0;
> +    ASSERT(!overflow && !underflow);
> +#endif

I know I raised the possibility of overflow. But I don’t think an assertion really helps much. The worry about overflow is it actually happening in the field, not happening on a debug build.

[WebKit Commit Bot]

Comment on attachment 80810 [details]
patch v3

Clearing flags on attachment: 80810

Committed r78652: <http://trac.webkit.org/changeset/78652>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.