Bug 90721 – Don't destroy the decoded data of an image if WebKit is about to render the image.

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

[Dongseong Hwang]

When the cache capacity of MemoryCache is exceeded, decoded data of all 
CachedImages are destroyed. Even the images inside the viewport are destroyed.
However, if the images need to be rendered again due to scoll events or
animation, they must be decoded again. As an extreme case, if there is an animation
with an image when MemoryCache is almost full, the image must be decoded every
frame. This slows down animation and needlessly consumes CPU cycles.

Therefore, it is better not to destory the decoded data of an image if the image
is inside the viewport because there is high chance that the image neeeds to be
rendered again soon. This patch reduce the unnecessary repetition of image decoding
on low memory, and also relieve the memory fragmentation because it avoids reallocation
of image frames.

In addition, there is another positive side effect. Currently,
CachedImageClient::willRenderImage() is used only to determine if GIF animation needs
to be paused or not in CachedImage::shouldPauseAnimation(). This patch makes
GIF animation outside the viewort be paused.

This is also a prerequisite for parallel image decoders. Because parallel image
decoders decode an image asynchronously, clients cannot render the image at the time
when the request is made. Clients can draw the image later after receiving image
decoding complete notification. However, there is a problem because MemoryCache can 
destroy the decoded data before clients actually render the image. So parallel image decoders
must prevent the decoded data from being destroyed if the image will be rendered
soon.

This patch may consume a little more memory, but furtunately the peak memory usage
is almost the same.

[Dongseong Hwang]

Created attachment 151136 [details]
Patch

[Alexey Proskuryakov]

CC'ing some folks who should have insight from earlier investigations.

[Simon Fraser (smfr)]

Comment on attachment 151136 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151136&action=review

> Source/WebCore/ChangeLog:8
> +        When the cache capacity of MemoryCache is exceeded, decoded data of all

... the MemoryCache
... the decoded data of all the CachedImages

> Source/WebCore/ChangeLog:15
> +        Therefore, it is better not to destory the decoded data of an image if the image

better to not destroy

> Source/WebCore/ChangeLog:37
> +        No new tests - no behavior changes. Only space-time tradeoff change.

There is a behavior change, which you note above. GIFs outside the viewport won't keep animating. Not sure if that's testable though.

> Source/WebCore/loader/cache/CachedImage.cpp:420
> +        CachedResourceClientWalker<CachedImageClient> w(m_clients);

Please use a longer variable name than 'w'.

> Source/WebCore/loader/cache/CachedImage.cpp:423
> +            if (c->willRenderImage(this))
> +                return;

It's odd that a method called destroyDecodedData() doesn't always do so. Maybe rename to destroyDecodedDataIfPossible(), or add CachedImage::likelyToRenderSoon() and call it somewhere.

[Dongseong Hwang]

Created attachment 151179 [details]
Patch

[Dongseong Hwang]

Thank Simon for your good advice.

I added CachedImage::likelyToUseSoon() to determine if live caches are removed, because I want to check this only in MemoryCache::pruneLiveResourcesToSize(), not in all the call sites of CachedImage::destroyDecodedData().
It means the previous patch had a bug and this patch fixes it.

[Simon Fraser (smfr)]

Comment on attachment 151179 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151179&action=review

Looks close, with some name tweaking.

> Source/WebCore/loader/cache/CachedResource.h:215
> +    virtual bool likelyToUseSoon() { return false; }

This should be const. I think "likelyToBeUsedSoon() const" would be better.

> Source/WebCore/loader/cache/MemoryCache.cpp:233
> +            // Check to see if the remaining resources are too high chance that the CachedResource uses soon to prune.

This comment doesn't make sense.

> Source/WebCore/loader/cache/MemoryCache.cpp:235
> +            if (current->likelyToUseSoon())
> +                return;

Why does this return, and not continue?

[Dongseong Hwang]

Created attachment 151353 [details]
Patch

[Dongseong Hwang]

(In reply to comment #6)
> (From update of attachment 151179 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151179&action=review
> 
> Looks close, with some name tweaking.
> 
> > Source/WebCore/loader/cache/CachedResource.h:215
> > +    virtual bool likelyToUseSoon() { return false; }
> 
> This should be const. I think "likelyToBeUsedSoon() const" would be better.

Done. I removed the argument CachedImage* of CacheImage::willRenderImage because I want to use willRenderImage in likelyToBeUsedSoon() "const" and the argument is not used anyway.

> 
> > Source/WebCore/loader/cache/MemoryCache.cpp:233
> > +            // Check to see if the remaining resources are too high chance that the CachedResource uses soon to prune.
> 
> This comment doesn't make sense.

I'm sorry. I clarified the comment.

> 
> > Source/WebCore/loader/cache/MemoryCache.cpp:235
> > +            if (current->likelyToUseSoon())
> > +                return;
> 
> Why does this return, and not continue?

Done. See the comment above.

[WebKit Review Bot]

Comment on attachment 151353 [details]
Patch

Attachment 151353 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13172282

New failing tests:
fast/images/gif-large-checkerboard.html
svg/carto.net/scrollbar.svg
svg/carto.net/selectionlist.svg

[WebKit Review Bot]

Created attachment 151372 [details]
Archive of layout-test-results from gce-cr-linux-06

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-06  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[WebKit Review Bot]

Comment on attachment 151353 [details]
Patch

Attachment 151353 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13177044

New failing tests:
fast/images/gif-large-checkerboard.html
svg/carto.net/scrollbar.svg
svg/carto.net/selectionlist.svg

[WebKit Review Bot]

Created attachment 151381 [details]
Archive of layout-test-results from gce-cr-linux-05

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Dongseong Hwang]

Created attachment 151389 [details]
Patch

[Dongseong Hwang]

(In reply to comment #13)
> Created an attachment (id=151389) [details]
> Patch

The layout tests failed because I forgot to update "current" in MemoryCache::pruneLiveResourcesToSize.

[Build Bot]

Comment on attachment 151389 [details]
Patch

Attachment 151389 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13168424

[Dongseong Hwang]

Created attachment 151394 [details]
Patch

[Dongseong Hwang]

(In reply to comment #16)
> Created an attachment (id=151394) [details]
> Patch

It is same to previous patch because mac-ews false failed.

[Dongseong Hwang]

Created attachment 151397 [details]
Patch

[Dongseong Hwang]

(In reply to comment #18)
> Created an attachment (id=151397) [details]
> Patch

Fix a typo in the Changelog.

[Antti Koivisto]

Comment on attachment 151397 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151397&action=review

Nice patch.

> Source/WebCore/loader/cache/CachedImage.h:133
> -    virtual bool willRenderImage(CachedImage*) { return false; }
> +    virtual bool willRenderImage() const { return false; }

I would keep the CachedImage* argument, even if currently unused, for consistency with other cache client calls. It also makes it clear that this is a cache client function (willRenderImage() on RenderObject could be anything otherwise) and could be useful for asserts.

[Kwang Yul Seo]

(In reply to comment #20)
> (From update of attachment 151397 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151397&action=review
> 
> Nice patch.
> 
> > Source/WebCore/loader/cache/CachedImage.h:133
> > -    virtual bool willRenderImage(CachedImage*) { return false; }
> > +    virtual bool willRenderImage() const { return false; }
> 
> I would keep the CachedImage* argument, even if currently unused, for consistency with other cache client calls. It also makes it clear that this is a cache client function (willRenderImage() on RenderObject could be anything otherwise) and could be useful for asserts.

Then CachedImage::likelyToBeUsedSoon() can't be const because it internally calls client->willRenderImage(this). 'this' pointer here is const CachedImage*, not CachedImage*.

I think it is better to drop const from CachedImage::likelyToBeUsedSoon() because another query method CachedImage::shouldPauseAnimation() is not const anyway.

If it is okay, I will land the patch for Dongsung after dropping const from CachedImage::likelyToBeUsedSoon().

[Antti Koivisto]

I don't think const makes sense in client interfaces anyway. Why should the interface determine what the client is allowed to do?

[Kwang Yul Seo]

Committed r122215: <http://trac.webkit.org/changeset/122215>

[Andreas Kling]

This change regressed Apple's Membuster benchmark by ~20% (80MB.)

Do you have any idea what may be causing this? Otherwise we should roll it out while we figure out the cause of the regression.

[Dongseong Hwang]

(In reply to comment #24)
> This change regressed Apple's Membuster benchmark by ~20% (80MB.)
> 
> Do you have any idea what may be causing this? Otherwise we should roll it out while we figure out the cause of the regression.

I'm sorry for this regression.

I don't know what you mean by "Apple's Membuster benchmark by ~20% (80MB.)"
I'm guessing that some of Apple's tests have a lot of images on the viewport, because the above patch does not remove decoded data of images when they lay on the viewport.

I need more information to know exactly why this patch caused the regression.

[Andreas Kling]

(In reply to comment #25)
> (In reply to comment #24)
> > This change regressed Apple's Membuster benchmark by ~20% (80MB.)
> > 
> > Do you have any idea what may be causing this? Otherwise we should roll it out while we figure out the cause of the regression.
> 
> I'm sorry for this regression.
> 
> I don't know what you mean by "Apple's Membuster benchmark by ~20% (80MB.)"
> I'm guessing that some of Apple's tests have a lot of images on the viewport, because the above patch does not remove decoded data of images when they lay on the viewport.
> 
> I need more information to know exactly why this patch caused the regression.

You can see the problem by opening a bunch of pages in tabs in Safari, then sending the low-memory signal like so:

notifyutil -p org.WebKit.lowMemory

With your change, there's significantly more memory used now, as the images "visible" in background tabs don't appear to get evicted.

[Dongseong Hwang]

(In reply to comment #26)
> You can see the problem by opening a bunch of pages in tabs in Safari, then sending the low-memory signal like so:
> 
> notifyutil -p org.WebKit.lowMemory
> 
> With your change, there's significantly more memory used now, as the images "visible" in background tabs don't appear to get evicted.

I feel very sorry for repeated problems from this patch.

Currently, this patch causes more memory consumption as well as pausing animated gif images (Bug 94874).

I think we should rollback this patch as others may think so. This patch landed 3 months ago, and we can not rollback using a simple git command.

I'll file another bug and patch the reverted one.

Thanks for reporting.

[Dongseong Hwang]

(In reply to comment #26)
> With your change, there's significantly more memory used now, as the images "visible" in background tabs don't appear to get evicted.

I posted the rollback patch in Bug 94874.

[Andreas Kling]

Thanks for rolling this out.

I investigated a bit, and found that applemac (at least) WebKit doesn't consider views as offscreen when they are in inactive tabs. This because ScrollView::isOffscreen() is not tab-aware. Page::isOnscreen() on the other hand is, and changing RenderObject::willRenderImage() to check that instead covered a large part of the memory usage regression in my tests.

[Stephanie Lewis]

Would that fix the version of the test that uses windows as well?