The default bug view has changed. See this FAQ.

Compositing step for WebGL readback allocates a gfxImageSurface per composite

RESOLVED FIXED in mozilla14

Status

()

Core
Canvas: WebGL
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jgilbert, Assigned: jgilbert)

Tracking

unspecified
mozilla14
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

5 years ago
Since the data involved is of size w*h*4, at 1080p this is over 8MB per frame, and, at 60fps, about 500MB/s in allocations/frees.

None of them last more than a frame, but maybe caching these temp surfaces would help our slow readback cases.
(Assignee)

Updated

5 years ago
OS: Windows 7 → All
Hardware: x86_64 → All
(Assignee)

Comment 1

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=b6e7039b8f2a

Will put up for review soon.
Assignee: nobody → jgilbert
Status: NEW → ASSIGNED
(Assignee)

Comment 2

5 years ago
Created attachment 608009 [details] [diff] [review]
Cache temporary surfaces used during canvas compositing

Also has a touch-up fix, while I was in the area.
Attachment #608009 - Flags: review?(bgirard)
Any idea how big the perf win is?
(Assignee)

Comment 4

5 years ago
(In reply to Benoit Girard (:BenWa) from comment #3)
> Any idea how big the perf win is?

Nope, but it must be better than thrashing memory (de)allocs.

I can try to check if it's noticeable, but I only have fairly modern machines, for which it shouldn't have much effect.

The biggest win should be on mobile, but I don't have an environment setup to test mobile yet.
Comment on attachment 608009 [details] [diff] [review]
Cache temporary surfaces used during canvas compositing

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

::: gfx/layers/basic/BasicLayers.cpp
@@ +1111,5 @@
> +      mCachedSize = aSize;
> +      mCachedFormat = aFormat;
> +    }
> +
> +    return mCachedTempSurface.forget();

Are you trying to retain the temporary surface? If so in this case I think you don't want to use .forget() since it will null out mCachedTempSurface:
http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsAutoPtr.h#1007

Why does this backend not get a Discard function like the other? It will get release when the layer is destroyed, I'm just wondering why this is different.

::: gfx/layers/d3d10/CanvasLayerD3D10.h
@@ +102,5 @@
> +  }
> +
> +  void DiscardTempBlob()
> +  {
> +      mCachedTempBlob = nsnull;

You clear the reference but it's not clear to me where this is delete[]-ed.
Attachment #608009 - Flags: review?(bgirard)
Normally 500MB/sec of malloc is noticeable on desktop I would think. I was curious to quantify if you had the data, but if you don't that fine. It's clearly a win.
(Assignee)

Comment 7

5 years ago
(In reply to Benoit Girard (:BenWa) from comment #5)
> Comment on attachment 608009 [details] [diff] [review]
> Cache temporary surfaces used during canvas compositing
> 
> Review of attachment 608009 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/basic/BasicLayers.cpp
> @@ +1111,5 @@
> > +      mCachedSize = aSize;
> > +      mCachedFormat = aFormat;
> > +    }
> > +
> > +    return mCachedTempSurface.forget();
> 
> Are you trying to retain the temporary surface? If so in this case I think
> you don't want to use .forget() since it will null out mCachedTempSurface:
> http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsAutoPtr.h#1007

Yes, my bad. Switching to nsAutoPtr, since that's all I wanted.

> 
> Why does this backend not get a Discard function like the other? It will get
> release when the layer is destroyed, I'm just wondering why this is
> different.
> 

I don't think the ones without the Discard function will change rendering path such that they will ever want to discard. I'll add Discard functions for these paths as well, though, to be safe(r) and consistent. 

> ::: gfx/layers/d3d10/CanvasLayerD3D10.h
> @@ +102,5 @@
> > +  }
> > +
> > +  void DiscardTempBlob()
> > +  {
> > +      mCachedTempBlob = nsnull;
> 
> You clear the reference but it's not clear to me where this is delete[]-ed.

nsAutoArrayPtr deletes the old array on reassignment.
(Assignee)

Updated

5 years ago
Attachment #608009 - Flags: review-
(Assignee)

Comment 8

5 years ago
Created attachment 608039 [details] [diff] [review]
Cache temporary surfaces used during canvas compositing
Attachment #608009 - Attachment is obsolete: true
Attachment #608039 - Flags: review?(bgirard)
(In reply to Jeff Gilbert [:jgilbert] from comment #7)
> nsAutoArrayPtr deletes the old array on reassignment.

Ohh opps, read the code wrong, that's fine.
Comment on attachment 608039 [details] [diff] [review]
Cache temporary surfaces used during canvas compositing

Thanks Jeff, useful patch.
Attachment #608039 - Flags: review?(bgirard) → review+
(Assignee)

Comment 11

5 years ago
Running try before pushing:
https://tbpl.mozilla.org/?tree=Try&rev=5a628dc158a2
Whiteboard: [rplus]
(Assignee)

Comment 12

5 years ago
Comment on attachment 608039 [details] [diff] [review]
Cache temporary surfaces used during canvas compositing

Memory management issues in BasicLayers and OGLLayers.
Attachment #608039 - Flags: review+ → review-
(Assignee)

Comment 13

5 years ago
Created attachment 608204 [details] [diff] [review]
Cache temporary surfaces used during canvas compositing

New patch, new try:
https://tbpl.mozilla.org/?tree=Try&rev=31adc3aac16f
Attachment #608039 - Attachment is obsolete: true
Attachment #608204 - Flags: review?(bgirard)
I see more leaks in your try push. Why is mCachedTempSurface no longer a nsRefPtr?
(Assignee)

Comment 15

5 years ago
(In reply to Benoit Girard (:BenWa) from comment #14)
> I see more leaks in your try push. Why is mCachedTempSurface no longer a
> nsRefPtr?

There are leaks but they are unrelated. (No gfxImageSurfaces or gfxASurfaces were leaked)

I fixed the mCachedTempSurface in OGLLayers, but looks like I forgot the one in BasicLayers.
(Assignee)

Comment 16

5 years ago
Created attachment 608375 [details] [diff] [review]
Cache temporary surfaces used during canvas compositing

https://tbpl.mozilla.org/?tree=Try&rev=865f7f3674eb
Attachment #608204 - Attachment is obsolete: true
Attachment #608375 - Flags: review?(bgirard)
Attachment #608204 - Flags: review?(bgirard)

Updated

5 years ago
Attachment #608375 - Flags: review?(bgirard) → review+
(Assignee)

Comment 17

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/636349fa2e09
Whiteboard: [rplus]
Target Milestone: --- → mozilla14
(Assignee)

Comment 18

5 years ago
Looking at the patch again, I see a place where, while the bare pointer should be fine, we should be using an nsRefPtr. This change was a holdover from my earlier iterations.
This patch was backed out by mbrubeck for android robocop failures.

https://hg.mozilla.org/integration/mozilla-inbound/rev/53c21294270d

There's a log with failures at https://tbpl.mozilla.org/php/getParsedLog.php?id=10296760&tree=Mozilla-Inbound&full=1 - it seems that intermittently, when the test first reads the pixels on the screen (using glReadPixels), it gets a white pixel in the top-left corner when that pixel should be black.
It looks like those failures are still happening even after the backout (see android robocop log for the mc -> mi merge at b3e429486693), so this is likely not caused by your patch.
(Assignee)

Comment 21

5 years ago
Created attachment 608868 [details] [diff] [review]
Cache temporary surfaces used during canvas compositing

Switching back to using nsRefPtr instead of a bare pointer. Carrying over r+.
Attachment #608375 - Attachment is obsolete: true
Attachment #608868 - Flags: review+
(Assignee)

Updated

5 years ago
Whiteboard: [rplus]
(Assignee)

Comment 22

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/5574b6eadb13
Whiteboard: [rplus]
https://hg.mozilla.org/mozilla-central/rev/5574b6eadb13
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Blocks: 774688
(Assignee)

Updated

5 years ago
Blocks: 784925
(Assignee)

Updated

5 years ago
No longer blocks: 784925
You need to log in before you can comment on or make changes to this bug.