Bug 53281 - To determine image properties, CG allocates memory which isn't included in CachedImage's decoded size
Summary: To determine image properties, CG allocates memory which isn't included in Ca...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Intel OS X 10.6
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-01-27 19:50 PST by Ian Henderson
Modified: 2015-05-22 10:43 PDT (History)
9 users (show)

See Also:


Attachments
1000-images test case (4.65 MB, application/zip)
2011-01-27 19:52 PST, Ian Henderson
no flags Details
proposed patch (1.85 KB, patch)
2011-01-27 20:01 PST, Ian Henderson
eric: review-
Details | Formatted Diff | Diff
track extra CG allocations as part of an image's decoded size (10.16 KB, patch)
2011-01-31 18:29 PST, Ian Henderson
darin: review-
darin: commit-queue-
Details | Formatted Diff | Diff
patch v3 (9.22 KB, patch)
2011-02-01 13:00 PST, Ian Henderson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ian Henderson 2011-01-27 19:50:58 PST
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.
Comment 1 Ian Henderson 2011-01-27 19:52:42 PST
Created attachment 80405 [details]
1000-images test case

Attaching 1000-images test case which exhibits this issue.
Comment 2 Ian Henderson 2011-01-27 20:01:35 PST
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%)
Comment 3 Andreas Kling 2011-01-28 03:27:49 PST
This doesn't cause the image to be (at least partially) decoded twice in the case where it's painted?
Comment 4 Eric Seidel (no email) 2011-01-28 12:00:20 PST
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.
Comment 5 Eric Seidel (no email) 2011-01-28 12:00:44 PST
Hyatt knows about the CG image decoders.  I'm not sure who else does.
Comment 6 Ian Henderson 2011-01-28 14:40:00 PST
(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.
Comment 7 David Kilzer (:ddkilzer) 2011-01-28 16:30:49 PST
<rdar://problem/8684763>
Comment 8 Ian Henderson 2011-01-31 18:29:09 PST
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.
Comment 9 Darin Adler 2011-02-01 09:52:18 PST
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.
Comment 10 Ian Henderson 2011-02-01 10:13:33 PST
Thanks for the review, Darin.  I've updated the title of the bug to describe the underlying issue rather than any particular fix.
Comment 11 Ian Henderson 2011-02-01 10:30:13 PST
(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?
Comment 12 Ian Henderson 2011-02-01 13:00:09 PST
Created attachment 80810 [details]
patch v3

Updated patch to address review comments.
Comment 13 Darin Adler 2011-02-15 13:25:36 PST
(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.
Comment 14 Darin Adler 2011-02-15 14:33:55 PST
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.
Comment 15 WebKit Commit Bot 2011-02-15 17:15:34 PST
Comment on attachment 80810 [details]
patch v3

Clearing flags on attachment: 80810

Committed r78652: <http://trac.webkit.org/changeset/78652>
Comment 16 WebKit Commit Bot 2011-02-15 17:15:49 PST
All reviewed patches have been landed.  Closing bug.