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.
Created attachment 80405 [details] 1000-images test case Attaching 1000-images test case which exhibits this issue.
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%)
This doesn't cause the image to be (at least partially) decoded twice in the case where it's painted?
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.
Hyatt knows about the CG image decoders. I'm not sure who else does.
(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.
<rdar://problem/8684763>
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 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.
Thanks for the review, Darin. I've updated the title of the bug to describe the underlying issue rather than any particular fix.
(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?
Created attachment 80810 [details] patch v3 Updated patch to address review comments.
(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 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 on attachment 80810 [details] patch v3 Clearing flags on attachment: 80810 Committed r78652: <http://trac.webkit.org/changeset/78652>
All reviewed patches have been landed. Closing bug.