We haven't been making good use of purgeable decoded image data, instead opting to throw out all the decoded data on a very frequent basis (sometimes even in between painting a single image into multiple tiles!). <rdar://problem/13205438>
This is primarily problematic for animated images, like GIFs.
Created attachment 218481 [details] take 1; needs more testing
Comment on attachment 218481 [details] take 1; needs more testing View in context: https://bugs.webkit.org/attachment.cgi?id=218481&action=review > Source/WebCore/platform/graphics/BitmapImage.cpp:512 > unsigned BitmapImage::decodedSize() const > { > + // If decoded data is purgeable, for the purposes of the memory > + // cache, pretend that it doesn't exist. > + if (decodedDataIsPurgeable()) > + return 0; It’s not 100% clear that this function is only suitable for use in the memory cache. What if it was used for some other purpose in the future? Might need to be renamed. > Source/WebCore/platform/graphics/BitmapImage.cpp:522 > +#if !USE(CG) > +bool BitmapImage::decodedDataIsPurgeable() const > +{ > + return false; > +} > +#endif Might be nice if this was inlined. > Source/WebCore/platform/graphics/cg/BitmapImageCG.cpp:195 > +bool BitmapImage::decodedDataIsPurgeable() const > +{ > +#if PLATFORM(MAC) > + return true; > +#else > + return false; > +#endif > +} Might be nice if this was inlined.
Comment on attachment 218481 [details] take 1; needs more testing View in context: https://bugs.webkit.org/attachment.cgi?id=218481&action=review > Source/WebCore/ChangeLog:13 > + Instead of throwing away decoded image data eagerly, allow the operating > + system to manage the memory via the standard purgeability mechanism. > + > + This improves the performance on some pathological cases (extremely large > + animated GIFs) by up to 8x. On reflection, the effectiveness of this probably depends on how well the purgeability mechanism works. We know it works very well for this purpose on Mavericks, but I believe it may not work as well on older versions of OS X or on various versions of iOS. So it’s dangerous to turn this on for PLATFORM(MAC) unconditionally.
(In reply to comment #4) > (From update of attachment 218481 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=218481&action=review > > > Source/WebCore/ChangeLog:13 > > + Instead of throwing away decoded image data eagerly, allow the operating > > + system to manage the memory via the standard purgeability mechanism. > > + > > + This improves the performance on some pathological cases (extremely large > > + animated GIFs) by up to 8x. > > On reflection, the effectiveness of this probably depends on how well the purgeability mechanism works. We know it works very well for this purpose on Mavericks, but I believe it may not work as well on older versions of OS X or on various versions of iOS. So it’s dangerous to turn this on for PLATFORM(MAC) unconditionally. That's a good point! Mavericks is where we were confident enough to turn on aggressive tile retention, too (which also depends heavily on purgeability), so maybe this should be keyed off OS X version (and tested on iOS).
(In reply to comment #3) > (From update of attachment 218481 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=218481&action=review > > > Source/WebCore/platform/graphics/BitmapImage.cpp:512 > > unsigned BitmapImage::decodedSize() const > > { > > + // If decoded data is purgeable, for the purposes of the memory > > + // cache, pretend that it doesn't exist. > > + if (decodedDataIsPurgeable()) > > + return 0; > > It’s not 100% clear that this function is only suitable for use in the memory cache. What if it was used for some other purpose in the future? Might need to be renamed. Additional logging and looking into this discovered that change did not have the intended effect (and that code was never called, so I removed it: https://bugs.webkit.org/show_bug.cgi?id=125327). So, we will still manually purge decoded data when the memory cache decides to (so, much bigger than 5MB, but still not using the full potential of purgeability), which usually happens under memory pressure. I have re-confirmed that purgeability is working by completely disabling the memory cache's ability to destroy the decoded data and repeating my original tests. I'll find a way to make this work given the new information.
Created attachment 218629 [details] take 2
Comment on attachment 218629 [details] take 2 View in context: https://bugs.webkit.org/attachment.cgi?id=218629&action=review > Source/WebCore/platform/graphics/Image.h:127 > + virtual bool isDecodedDataPurgeable() const { return false; } decodedDataIsPurgeable() ?
Created attachment 218630 [details] patch for landing with new name
Comment on attachment 218630 [details] patch for landing with new name Clearing flags on attachment: 218630 Committed r160260: <http://trac.webkit.org/changeset/160260>
All reviewed patches have been landed. Closing bug.
reopen to fix a IRC review comment from mitz
Created attachment 218635 [details] fix review comment
Comment on attachment 218635 [details] fix review comment Rejecting attachment 218635 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 218635, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit /Volumes/Data/EWS/WebKit/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://webkit-queues.appspot.com/results/46038029
Created attachment 218636 [details] make the cq happy :(
Comment on attachment 218636 [details] make the cq happy :( Clearing flags on attachment: 218636 Committed r160265: <http://trac.webkit.org/changeset/160265>
Reverted in http://trac.webkit.org/changeset/160943.
Comment on attachment 218481 [details] take 1; needs more testing Cleared Darin Adler's review+ from obsolete attachment 218481 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment on attachment 218629 [details] take 2 Cleared Simon Fraser's review+ from obsolete attachment 218629 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Committed r176106: <http://trac.webkit.org/changeset/176106>
(In reply to comment #21) > Committed r176106: <http://trac.webkit.org/changeset/176106> Would there be any value in adding/is it possible to write a performance test for this change?
Comment on attachment 218630 [details] patch for landing with new name View in context: https://bugs.webkit.org/attachment.cgi?id=218630&action=review > Source/WebCore/platform/graphics/BitmapImage.h:265 > +#if PLATFORM(MAC) && !PLATFORM(IOS) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 1090 No need for “!PLATFORM(IOS)” since PLATFORM(MAC) and PLATFORM(IOS) are mutually exclusive now.
Comment on attachment 218630 [details] patch for landing with new name View in context: https://bugs.webkit.org/attachment.cgi?id=218630&action=review >> Source/WebCore/platform/graphics/BitmapImage.h:265 >> +#if PLATFORM(MAC) && !PLATFORM(IOS) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 1090 > > No need for “!PLATFORM(IOS)” since PLATFORM(MAC) and PLATFORM(IOS) are mutually exclusive now. You can tell how long this patch has been sitting on my disk :)
Re-opened since this is blocked by bug 138719
Created attachment 241521 [details] Patch for Ews...
Attachment 241521 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/BitmapImage.h:287: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WebCore/platform/graphics/cg/ImageSourceCG.cpp:54: "CoreGraphics/CGImagePrivate.h" already included at Source/WebCore/platform/graphics/cg/ImageSourceCG.cpp:42 [build/include] [4] ERROR: Source/WebCore/platform/graphics/cg/ImageSourceCG.cpp:57: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/graphics/cg/ImageSourceCG.cpp:58: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Total errors found: 4 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 241521 [details] Patch for Ews... Clearing flags on attachment: 241521 Committed r176115: <http://trac.webkit.org/changeset/176115>