Bug 67899 - Make sure to GC decoded images that are only used with WebGL
Summary: Make sure to GC decoded images that are only used with WebGL
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: John Bauman
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-09-10 18:01 PDT by John Bauman
Modified: 2011-09-14 19:11 PDT (History)
3 users (show)

See Also:


Attachments
Patch (1.64 KB, patch)
2011-09-10 18:03 PDT, John Bauman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Bauman 2011-09-10 18:01:38 PDT
Make sure to GC decoded images that are only used with WebGL
Comment 1 John Bauman 2011-09-10 18:03:54 PDT
Created attachment 106994 [details]
Patch
Comment 2 John Bauman 2011-09-10 18:09:07 PDT
See http://code.google.com/p/chromium/issues/detail?id=94444 for why this is useful. With the patch, the amount of memory used only barely goes up with time, instead of causing the renderer to run out of address space

I'm not sure why adding didDraw is necessary for the image to be collected. There are a bunch of ways to use an image that may not cause that method to be called, so it might be nice to have some other way to GC them.
Comment 3 Kenneth Russell 2011-09-12 12:10:07 PDT
Nice catch.

Could you instrument CachedResource::didAccessDecodedData and see which code path is taken for the images in question? It would be good to know exactly what interaction is going on with the memory cache -- in particular, whether the image is in the cache (presumably it is) and whether it's considered to be live.
Comment 4 James Robinson 2011-09-12 17:43:21 PDT
didDraw() kicks the memory cache and (potentially) triggers eviction, so it's possible on your test case that there's nothing else going on, there's nothing to trigger MemoryCache::prune() other than the didDraw call from WebGL.
Comment 5 John Bauman 2011-09-12 21:29:53 PDT
Yeah, looks like MemoryCache::prune is never being called, because the app only loads images and does WebGL rendering, neither of which cause that. It might be nice to also do pruning when images are loaded in case some other path forgets to do didDraw, though this patch is still valuable in updating the LRU list.
Comment 6 James Robinson 2011-09-13 10:09:15 PDT
Comment on attachment 106994 [details]
Patch

R=me
Comment 7 John Bauman 2011-09-14 07:18:58 PDT
Comment on attachment 106994 [details]
Patch

Oops, forgot to set that it needs commit queue. James?
Comment 8 WebKit Review Bot 2011-09-14 10:08:34 PDT
Comment on attachment 106994 [details]
Patch

Rejecting attachment 106994 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
 name: third_party/ots
    url: From('chromium_deps', 'src/third_party/ots')
    should_process: True
    requirements: set(['third_party', 'chromium_deps', './'])
  
    name: third_party/snappy/src
    url: From('chromium_deps', 'src/third_party/snappy/src')
    should_process: True
    requirements: set(['third_party', 'chromium_deps', './'])

Died at Tools/Scripts/update-webkit-chromium line 80.
No such file or directory at /mnt/git/webkit-commit-queue/Tools/Scripts/webkitdirs.pm line 1929.

Full output: http://queues.webkit.org/results/9661518
Comment 9 Kenneth Russell 2011-09-14 11:15:23 PDT
Comment on attachment 106994 [details]
Patch

Looks like a flake. Let's try that again.
Comment 10 WebKit Review Bot 2011-09-14 19:11:41 PDT
Comment on attachment 106994 [details]
Patch

Clearing flags on attachment: 106994

Committed r95150: <http://trac.webkit.org/changeset/95150>
Comment 11 WebKit Review Bot 2011-09-14 19:11:46 PDT
All reviewed patches have been landed.  Closing bug.