Bug 125273 – [mac] Keep around more decoded image data, since it's purgeable

• Status: RESOLVED
• Resolution: FIXED
• Component: Images

[Tim Horton]

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>

[Tim Horton]

This is primarily problematic for animated images, like GIFs.

[Tim Horton]

Created attachment 218481 [details]
take 1; needs more testing

[Darin Adler]

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.

[Darin Adler]

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.

[Tim Horton]

(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).

[Tim Horton]

(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.

[Tim Horton]

Created attachment 218629 [details]
take 2

[Simon Fraser (smfr)]

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() ?

[Tim Horton]

Created attachment 218630 [details]
patch for landing with new name

[WebKit Commit Bot]

Comment on attachment 218630 [details]
patch for landing with new name

Clearing flags on attachment: 218630

Committed r160260: <http://trac.webkit.org/changeset/160260>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Tim Horton]

reopen to fix a IRC review comment from mitz

[Tim Horton]

Created attachment 218635 [details]
fix review comment

[WebKit Commit Bot]

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

[Tim Horton]

Created attachment 218636 [details]
make the cq happy :(

[WebKit Commit Bot]

Comment on attachment 218636 [details]
make the cq happy :(

Clearing flags on attachment: 218636

Committed r160265: <http://trac.webkit.org/changeset/160265>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Tim Horton]

Reverted in http://trac.webkit.org/changeset/160943.

[Csaba Osztrogonác]

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.

[Csaba Osztrogonác]

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.

[Andreas Kling]

Committed r176106: <http://trac.webkit.org/changeset/176106>

[Daniel Bates]

(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.

[Tim Horton]

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 :)

[WebKit Commit Bot]

Re-opened since this is blocked by bug 138719

[Andreas Kling]

Created attachment 241521 [details]
Patch for Ews...

[WebKit Commit Bot]

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.

[WebKit Commit Bot]

Comment on attachment 241521 [details]
Patch for Ews...

Clearing flags on attachment: 241521

Committed r176115: <http://trac.webkit.org/changeset/176115>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.