Last Comment Bug 809587 - canvas.toBlob() doesn't increment the JS malloc counter
: canvas.toBlob() doesn't increment the JS malloc counter
Status: RESOLVED FIXED
[MemShrink][soft-blocker]
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla19
Assigned To: Justin Lebar (not reading bugmail)
:
Mentors:
: 802662 (view as bug list)
Depends on:
Blocks: slim-fast 648610 798002
  Show dependency treegraph
 
Reported: 2012-11-07 12:43 PST by Justin Lebar (not reading bugmail)
Modified: 2012-11-09 15:42 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
fixed


Attachments
Patch, v1 (1.15 KB, patch)
2012-11-07 15:04 PST, Justin Lebar (not reading bugmail)
no flags Details | Diff | Review
Patch, v2 (2.08 KB, patch)
2012-11-07 15:13 PST, Justin Lebar (not reading bugmail)
no flags Details | Diff | Review
Patch v3 (1.80 KB, patch)
2012-11-07 17:16 PST, Justin Lebar (not reading bugmail)
khuey: review+
Details | Diff | Review

Description Justin Lebar (not reading bugmail) 2012-11-07 12:43:33 PST
There's a JS malloc counter which we should increment every time we do a big allocation; it affects how often the GC gets run.

Are we incrementing this counter when we take a blob screenshot (i.e., when we do canvas.toBlob)?  If not, we should be.
Comment 1 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-11-07 13:14:54 PST
They do not.
Comment 2 Justin Lebar (not reading bugmail) 2012-11-07 15:04:21 PST
Created attachment 679397 [details] [diff] [review]
Patch, v1
Comment 3 Justin Lebar (not reading bugmail) 2012-11-07 15:08:34 PST
Comment on attachment 679397 [details] [diff] [review]
Patch, v1

Heh, I didn't actually compile this

> +  if (context) {

if (cx)
Comment 4 Justin Lebar (not reading bugmail) 2012-11-07 15:13:20 PST
Created attachment 679400 [details] [diff] [review]
Patch, v2

Increment the malloc counter in one other place.  This covers all instances of nsDOMMemoryFile, which I guess is what we want?
Comment 5 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-11-07 17:12:30 PST
Comment on attachment 679400 [details] [diff] [review]
Patch, v2

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

::: content/html/content/src/nsHTMLCanvasElement.cpp
@@ +682,5 @@
>  
> +  JSContext* cx = nsContentUtils::GetCurrentJSContext();
> +  if (cx) {
> +    JS_updateMallocCounter(cx, imgSize);
> +  }

Why are you doing this twice here?
Comment 6 Justin Lebar (not reading bugmail) 2012-11-07 17:15:24 PST
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #5)
> Comment on attachment 679400 [details] [diff] [review]
> Patch, v2
> 
> Review of attachment 679400 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/html/content/src/nsHTMLCanvasElement.cpp
> @@ +682,5 @@
> >  
> > +  JSContext* cx = nsContentUtils::GetCurrentJSContext();
> > +  if (cx) {
> > +    JS_updateMallocCounter(cx, imgSize);
> > +  }
> 
> Why are you doing this twice here?

Because I was writing a code during a meeting.
Comment 7 Justin Lebar (not reading bugmail) 2012-11-07 17:16:32 PST
Created attachment 679461 [details] [diff] [review]
Patch v3

Now with less less repetition.
Comment 8 Justin Lebar (not reading bugmail) 2012-11-07 17:46:59 PST
Soft basecamp blocker because this should help us purge screenshots out of the heap more quickly and because it's a low-risk change.
Comment 9 Justin Lebar (not reading bugmail) 2012-11-07 17:53:15 PST
Thanks, Kyle.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/284ad66e8bda
Comment 10 Ed Morley [:emorley] 2012-11-08 02:38:58 PST
https://hg.mozilla.org/mozilla-central/rev/284ad66e8bda
Comment 11 Ryan VanderMeulen [:RyanVM] 2012-11-08 12:41:37 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/bbdbb8b0cef8
Comment 12 Justin Lebar (not reading bugmail) 2012-11-09 15:42:33 PST
*** Bug 802662 has been marked as a duplicate of this bug. ***

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