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

RESOLVED FIXED in Firefox 18

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Justin Lebar (not reading bugmail), Assigned: Justin Lebar (not reading bugmail))

Tracking

(Blocks: 1 bug)

Trunk
mozilla19
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed)

Details

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

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Updated

5 years ago
Blocks: 797189
(Assignee)

Updated

5 years ago
Whiteboard: [MemShrink]
They do not.
(Assignee)

Updated

5 years ago
Summary: Does taking blob screenshots increment the JS malloc counter? → canvas.toBlob() doesn't increment the JS malloc counter
(Assignee)

Comment 2

5 years ago
Created attachment 679397 [details] [diff] [review]
Patch, v1
Attachment #679397 - Flags: review?(khuey)
(Assignee)

Comment 3

5 years ago
Comment on attachment 679397 [details] [diff] [review]
Patch, v1

Heh, I didn't actually compile this

> +  if (context) {

if (cx)
(Assignee)

Comment 4

5 years ago
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?
Attachment #679397 - Attachment is obsolete: true
Attachment #679397 - Flags: review?(khuey)
Attachment #679400 - Flags: review?(khuey)
(Assignee)

Updated

5 years ago
Assignee: nobody → justin.lebar+bug
(Assignee)

Updated

5 years ago
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?
(Assignee)

Comment 6

5 years ago
(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.
(Assignee)

Comment 7

5 years ago
Created attachment 679461 [details] [diff] [review]
Patch v3

Now with less less repetition.
Attachment #679400 - Attachment is obsolete: true
Attachment #679400 - Flags: review?(khuey)
Attachment #679461 - Flags: review?(khuey)
Attachment #679461 - Flags: review?(khuey) → review+
(Assignee)

Comment 8

5 years ago
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]
(Assignee)

Comment 9

5 years ago
Thanks, Kyle.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/284ad66e8bda
https://hg.mozilla.org/mozilla-central/rev/284ad66e8bda
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
https://hg.mozilla.org/releases/mozilla-aurora/rev/bbdbb8b0cef8
status-firefox18: --- → fixed
status-firefox19: --- → fixed
(Assignee)

Updated

5 years ago
Duplicate of this bug: 802662
You need to log in before you can comment on or make changes to this bug.