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.
Created attachment 151136 [details] Patch
CC'ing some folks who should have insight from earlier investigations.
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.
Created attachment 151179 [details] Patch
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.
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?
Created attachment 151353 [details] Patch
(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.
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
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
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
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
Created attachment 151389 [details] Patch
(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.
Comment on attachment 151389 [details] Patch Attachment 151389 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13168424
Created attachment 151394 [details] Patch
(In reply to comment #16) > Created an attachment (id=151394) [details] > Patch It is same to previous patch because mac-ews false failed.
Created attachment 151397 [details] Patch
(In reply to comment #18) > Created an attachment (id=151397) [details] > Patch Fix a typo in the Changelog.
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.
(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().
I don't think const makes sense in client interfaces anyway. Why should the interface determine what the client is allowed to do?
Committed r122215: <http://trac.webkit.org/changeset/122215>
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.
(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.
(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.
(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.
(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.
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.
Would that fix the version of the test that uses windows as well?