Closed Bug 809587 Opened 8 years ago Closed 8 years ago

canvas.toBlob() doesn't increment the JS malloc counter

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla19
blocking-basecamp +
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)

References

Details

(Whiteboard: [MemShrink][soft-blocker])

Attachments

(1 file, 2 obsolete files)

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.
Blocks: slim-fast
Whiteboard: [MemShrink]
Summary: Does taking blob screenshots increment the JS malloc counter? → canvas.toBlob() doesn't increment the JS malloc counter
Attached patch Patch, v1 (obsolete) — Splinter Review
Attachment #679397 - Flags: review?(khuey)
Comment on attachment 679397 [details] [diff] [review]
Patch, v1

Heh, I didn't actually compile this

> +  if (context) {

if (cx)
Attached patch Patch, v2 (obsolete) — Splinter Review
Increment the malloc counter in one other place.  This covers all instances of nsDOMMemoryFile, which I guess is what we want?
Attachment #679397 - Attachment is obsolete: true
Attachment #679397 - Flags: review?(khuey)
Attachment #679400 - Flags: review?(khuey)
Assignee: nobody → justin.lebar+bug
Blocks: 798002
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?
(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.
Attached patch Patch v3Splinter Review
Now with less less repetition.
Attachment #679400 - Attachment is obsolete: true
Attachment #679400 - Flags: review?(khuey)
Attachment #679461 - Flags: review?(khuey)
Soft basecamp blocker because this should help us purge screenshots out of the heap more quickly and because it's a low-risk change.
blocking-basecamp: --- → +
Whiteboard: [MemShrink] → [MemShrink][soft-blocker]
https://hg.mozilla.org/mozilla-central/rev/284ad66e8bda
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Duplicate of this bug: 802662
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.