Summary: | Don't destroy the decoded data of an image if WebKit is about to render the image. | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dongseong Hwang <dongseong.hwang> | ||||||||||||||||||
Component: | Images | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | aestes, ap, dev+webkit, dglazkov, eric, gram, japhet, jochen, kling, koivisto, laszlo.gombos, simon.fraser, skyul, slewis, webkit.review.bot | ||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||
Bug Blocks: | 90375 | ||||||||||||||||||||
Attachments: |
|
Description
Dongseong Hwang
2012-07-07 00:08:43 PDT
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? |