Last Comment Bug 865929 - Add Pref to limit the size of the canvas image cache
: Add Pref to limit the size of the canvas image cache
Status: RESOLVED FIXED
[MemShrink][fixed-in-birch]
:
Product: Core
Classification: Components
Component: Canvas: 2D (show other bugs)
: unspecified
: x86 Mac OS X
-- normal (vote)
: mozilla23
Assigned To: Andreas Gal :gal
:
: Milan Sreckovic [:milan]
Mentors:
Depends on:
Blocks: 854799 861965
  Show dependency treegraph
 
Reported: 2013-04-25 17:18 PDT by Andreas Gal :gal
Modified: 2013-08-01 07:45 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
tef+
wontfix
wontfix
verified
fixed
wontfix
fixed


Attachments
patch (3.55 KB, patch)
2013-04-25 17:18 PDT, Andreas Gal :gal
no flags Details | Diff | Splinter Review
updated patch (4.15 KB, patch)
2013-04-25 17:36 PDT, Andreas Gal :gal
no flags Details | Diff | Splinter Review
patch (4.19 KB, patch)
2013-04-25 17:42 PDT, Andreas Gal :gal
justin.lebar+bug: review+
Details | Diff | Splinter Review
properly update cache size, and avoid shutdown crash (5.06 KB, patch)
2013-04-25 21:14 PDT, Andreas Gal :gal
no flags Details | Diff | Splinter Review
patch, minus redundant memory pressure purging (3.50 KB, patch)
2013-04-26 06:47 PDT, Andreas Gal :gal
josh: review+
Details | Diff | Splinter Review

Description User image Andreas Gal :gal 2013-04-25 17:18:03 PDT

    
Comment 1 User image Andreas Gal :gal 2013-04-25 17:18:34 PDT
Created attachment 742102 [details] [diff] [review]
patch
Comment 2 User image Andreas Gal :gal 2013-04-25 17:19:18 PDT
Blocks a TEF+ blocker, so this one is TEF+ too.
Comment 3 User image Andreas Gal :gal 2013-04-25 17:26:53 PDT
mSize should be set to 0 in the constructor, I updated the patch to do so
Comment 4 User image Andreas Gal :gal 2013-04-25 17:36:33 PDT
Created attachment 742111 [details] [diff] [review]
updated patch
Comment 5 User image Justin Lebar (not reading bugmail) 2013-04-25 17:39:57 PDT
> +  // Expire the image cache early if its larger than we want it to be.
> +  while (gImageCache->mSize > size_t(sCanvasImageCacheLimit))
> +    gImageCache->AgeOneGeneration();

With sCanvasImageCacheLimit set to 0 outside b2g, I think this disables the cache.
Comment 6 User image Andreas Gal :gal 2013-04-25 17:40:45 PDT
right, lemme fix that
Comment 7 User image Andreas Gal :gal 2013-04-25 17:42:19 PDT
Created attachment 742113 [details] [diff] [review]
patch
Comment 9 User image David Flanagan [:djf] 2013-04-25 19:03:41 PDT
When I apply this patch to b2g-18 and then remove the 3-second timeout in bug 861965, I see two things:

1) When I start gallery the first time, it scans small images, but the first time it comes to a big ~5mp image, the scanning process hangs.  Something in the process is not completing.  Given the nature of the patch I'm assuming it is the call to drawImage or the subsequent call to canvas.toBlob().  So the gallery hangs while scanning, and the UX is non-responsive: I can't tap on one of the already-scanned thumbnails to view it.

2) If I kill and restart the gallery, then I see the first of the 5mp images, and then it OOMs (but does not actually crash as I was seeing on m-c).

When I modified CanvasImageCache::NotifyDrawImage so it returned early, that did solve my bug, so I think we're on the right track here.
Comment 11 User image David Flanagan [:djf] 2013-04-25 19:22:26 PDT
I've investigated more carefully what's happening in the gallery app.

When I run with this patch, the first large image is processed successfully, except that the thumbnail does not get displayed.  (I don't know if the thumbnail is displayed but is blank or if it just doesn't get drawn.)

Its the second large image where scanning hangs.  And it hangs in the call to canvas.toBlob() where I'm trying to extract a jpeg blob from a canvas I've just done a drawImage() to.
Comment 12 User image David Flanagan [:djf] 2013-04-25 19:27:16 PDT
If I add a 10ms setTimeout() between large images, nothing changes.

If I make it 1000ms, I get an OOM on the first run rather than blocking in the toBlob() call.
Comment 13 User image Ryan VanderMeulen [:RyanVM] 2013-04-25 19:41:52 PDT
Err, why was this resolved when it hit inbound? Also, isn't B2G work supposed to be landing on birch?
Comment 14 User image Andreas Gal :gal 2013-04-25 20:46:04 PDT
I definitely caused that crash.
Comment 15 User image Andreas Gal :gal 2013-04-25 21:14:55 PDT
Created attachment 742184 [details] [diff] [review]
properly update cache size, and avoid shutdown crash
Comment 16 User image Justin Lebar (not reading bugmail) 2013-04-25 21:55:46 PDT
To make sure I understand this: If you'd changed the observer to be a weak ref, would that also have solved the shutdown crashes?

I shouldn't have let you get away with those non-virtual overrides.  :-/
Comment 17 User image David Flanagan [:djf] 2013-04-25 22:19:58 PDT
I don't know anything about all about the canvas image cache, but I'm guessing that it holds on to images passed to drawImage() so that we can pretend that drawImage() is a synchronous function.

Currently my gallery code is doing just that: it calls drawImage(), then sets img.src = '' to release the image memory and then calls canvas.toBlob().  So presumably I'm releasing image memory before the draw is done.  And if this patch doesn't cache my image because it is so big, what is going to happen?  Does some other kind of image locking take care of this case, or is it possible that bad things will happen here?

Obviously, I can change the gallery app to only release the image once toBlob() has called its callback, but we can't mandate that all code on the web does that...

For large images that we are not going to store in the canvas image cache, can we make drawImage() synchronous?
Comment 18 User image David Flanagan [:djf] 2013-04-25 22:28:07 PDT
This latest version of the patch fixes the issue for me in bug 861965.  (To be on the safe side, I've also changed my code to free the image after creating the blob.)
Comment 19 User image Justin Lebar (not reading bugmail) 2013-04-25 22:35:14 PDT
MemShrink doesn't pre-prioritize bugs (as e.g. snappy does), because we like to look over the list of new, untriaged bugs at our meetings.
Comment 20 User image Josh Matthews [:jdm] 2013-04-26 03:25:50 PDT
For what it's worth, any class that inherits from nsExpirationTracker automatically observes memory-pressure, so the change here should be redundant: http://mxr.mozilla.org/mozilla-central/source/xpcom/ds/nsExpirationTracker.h#286
Comment 21 User image Andreas Gal :gal 2013-04-26 06:44:35 PDT
jdm, cool, I will remove those parts
Comment 22 User image Andreas Gal :gal 2013-04-26 06:47:59 PDT
Created attachment 742347 [details] [diff] [review]
patch, minus redundant memory pressure purging
Comment 23 User image Josh Matthews [:jdm] 2013-04-26 06:56:23 PDT
Comment on attachment 742347 [details] [diff] [review]
patch, minus redundant memory pressure purging

Review of attachment 742347 [details] [diff] [review]:
-----------------------------------------------------------------

This r+ is just approving the removal of the redundant code; I know nothing about CanvasImageCache so it shouldn't be taken as a blessing of the remaining changes.
Comment 24 User image Andreas Gal :gal 2013-04-26 07:04:49 PDT
jdm reviewed the removal, jlebar already reviewed the canvasimagecache changes
Comment 25 User image Andreas Gal :gal 2013-04-26 07:17:36 PDT
http://hg.mozilla.org/projects/birch/rev/52a6a51d2dca
Comment 26 User image Andreas Gal :gal 2013-04-26 07:46:39 PDT
comment 17: drawImage does a sync decode if needed, so I think your code is fine. The image cache is strictly to optimize the case where you do two subsequent drawImage calls with the same image within 1-2 seconds (games animating sprites).
Comment 27 User image David Flanagan [:djf] 2013-04-26 09:43:33 PDT
Good to know that I don't have to worry about the cache.

The current version of the patch still fixes my gallery issue. Thanks!
Comment 28 User image Ryan VanderMeulen [:RyanVM] 2013-04-26 13:45:57 PDT
https://hg.mozilla.org/mozilla-central/rev/52a6a51d2dca
Comment 30 User image Jim Jeffery not reading bug-mail 1/2/11 2013-04-30 16:54:25 PDT
Sorry for bug spam, but I and confused whether this bug is actually in m-c builds.

Looking tbpl it appears it landed in m-c under
cset: https://hg.mozilla.org/mozilla-central/rev/ccdc2beb09ae 

then backed out in cset:
https://hg.mozilla.org/mozilla-central/rev/7338d59869c3

It seems to be that it landed in m-c from a birch->m-c merge as noted in comment #28
then as I noted above it landed in m-c with a different cset, then backed out...

Sorry, color me confused - if this has indeed landed correctly in m-c the nevermind me, but seems from what I see this bug is not in m-c and should be re-opened >?
Comment 31 User image Justin Lebar (not reading bugmail) 2013-04-30 17:00:16 PDT
Seems like it's currently in tip of trunk.

https://github.com/mozilla/mozilla-central/blob/HEAD/b2g/app/b2g.js#L31
Comment 32 User image Ioana (away) 2013-08-01 07:45:35 PDT
The pref is present in Firefox 23 RC - 20130730113002, with the default value '0'.

Note You need to log in before you can comment on or make changes to this bug.