Closed Bug 735378 Opened 10 years ago Closed 10 years ago

Compositing step for WebGL readback allocates a gfxImageSurface per composite

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: jgilbert, Assigned: jgilbert)

References

Details

Attachments

(1 file, 4 obsolete files)

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.
OS: Windows 7 → All
Hardware: x86_64 → All
https://tbpl.mozilla.org/?tree=Try&rev=b6e7039b8f2a

Will put up for review soon.
Assignee: nobody → jgilbert
Status: NEW → ASSIGNED
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?
(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.
(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.
Attachment #608009 - Flags: review-
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+
Running try before pushing:
https://tbpl.mozilla.org/?tree=Try&rev=5a628dc158a2
Whiteboard: [rplus]
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-
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?
(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.
https://tbpl.mozilla.org/?tree=Try&rev=865f7f3674eb
Attachment #608204 - Attachment is obsolete: true
Attachment #608375 - Flags: review?(bgirard)
Attachment #608204 - Flags: review?(bgirard)
Attachment #608375 - Flags: review?(bgirard) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/636349fa2e09
Whiteboard: [rplus]
Target Milestone: --- → mozilla14
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.
Switching back to using nsRefPtr instead of a bare pointer. Carrying over r+.
Attachment #608375 - Attachment is obsolete: true
Attachment #608868 - Flags: review+
Whiteboard: [rplus]
https://hg.mozilla.org/mozilla-central/rev/5574b6eadb13
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Blocks: 774688
Blocks: 784925
No longer blocks: 784925
You need to log in before you can comment on or make changes to this bug.