WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
53281
To determine image properties, CG allocates memory which isn't included in CachedImage's decoded size
https://bugs.webkit.org/show_bug.cgi?id=53281
Summary
To determine image properties, CG allocates memory which isn't included in Ca...
Ian Henderson
Reported
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.
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
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Ian Henderson
Comment 1
2011-01-27 19:52:42 PST
Created
attachment 80405
[details]
1000-images test case Attaching 1000-images test case which exhibits this issue.
Ian Henderson
Comment 2
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%)
Andreas Kling
Comment 3
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?
Eric Seidel (no email)
Comment 4
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.
Eric Seidel (no email)
Comment 5
2011-01-28 12:00:44 PST
Hyatt knows about the CG image decoders. I'm not sure who else does.
Ian Henderson
Comment 6
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.
David Kilzer (:ddkilzer)
Comment 7
2011-01-28 16:30:49 PST
<
rdar://problem/8684763
>
Ian Henderson
Comment 8
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.
Darin Adler
Comment 9
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.
Ian Henderson
Comment 10
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.
Ian Henderson
Comment 11
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?
Ian Henderson
Comment 12
2011-02-01 13:00:09 PST
Created
attachment 80810
[details]
patch v3 Updated patch to address review comments.
Darin Adler
Comment 13
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.
Darin Adler
Comment 14
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.
WebKit Commit Bot
Comment 15
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
>
WebKit Commit Bot
Comment 16
2011-02-15 17:15:49 PST
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug