Closed Bug 809587 Opened 11 years ago Closed 11 years ago

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

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla19
Project Flags:
blocking-basecamp +
Tracking Flags:
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)

Patch v3
1.80 KB, patch
khuey
: review+
Details | Diff | Splinter Review 
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 v3
        — 
          — Splinter 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: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Component: DOM → DOM: Core & HTML

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