Last Comment Bug 735378 - Compositing step for WebGL readback allocates a gfxImageSurface per composite
: Compositing step for WebGL readback allocates a gfxImageSurface per composite
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Canvas: WebGL (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla14
Assigned To: Jeff Gilbert [:jgilbert]
:
Mentors:
Depends on:
Blocks: 774688
  Show dependency treegraph
 
Reported: 2012-03-13 12:12 PDT by Jeff Gilbert [:jgilbert]
Modified: 2012-08-22 22:09 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Cache temporary surfaces used during canvas compositing (12.18 KB, patch)
2012-03-21 10:24 PDT, Jeff Gilbert [:jgilbert]
jgilbert: review-
Details | Diff | Splinter Review
Cache temporary surfaces used during canvas compositing (12.78 KB, patch)
2012-03-21 11:41 PDT, Jeff Gilbert [:jgilbert]
jgilbert: review-
Details | Diff | Splinter Review
Cache temporary surfaces used during canvas compositing (13.29 KB, patch)
2012-03-21 19:50 PDT, Jeff Gilbert [:jgilbert]
no flags Details | Diff | Splinter Review
Cache temporary surfaces used during canvas compositing (12.72 KB, patch)
2012-03-22 10:21 PDT, Jeff Gilbert [:jgilbert]
b56girard: review+
Details | Diff | Splinter Review
Cache temporary surfaces used during canvas compositing (12.72 KB, patch)
2012-03-23 14:32 PDT, Jeff Gilbert [:jgilbert]
jgilbert: review+
Details | Diff | Splinter Review

Description Jeff Gilbert [:jgilbert] 2012-03-13 12:12:56 PDT
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.
Comment 1 Jeff Gilbert [:jgilbert] 2012-03-21 02:04:42 PDT
https://tbpl.mozilla.org/?tree=Try&rev=b6e7039b8f2a

Will put up for review soon.
Comment 2 Jeff Gilbert [:jgilbert] 2012-03-21 10:24:05 PDT
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.
Comment 3 Benoit Girard (:BenWa) 2012-03-21 10:25:41 PDT
Any idea how big the perf win is?
Comment 4 Jeff Gilbert [:jgilbert] 2012-03-21 10:31:50 PDT
(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 5 Benoit Girard (:BenWa) 2012-03-21 10:35:14 PDT
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.
Comment 6 Benoit Girard (:BenWa) 2012-03-21 10:37:21 PDT
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.
Comment 7 Jeff Gilbert [:jgilbert] 2012-03-21 11:40:28 PDT
(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.
Comment 8 Jeff Gilbert [:jgilbert] 2012-03-21 11:41:50 PDT
Created attachment 608039 [details] [diff] [review]
Cache temporary surfaces used during canvas compositing
Comment 9 Benoit Girard (:BenWa) 2012-03-21 11:48:26 PDT
(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 10 Benoit Girard (:BenWa) 2012-03-21 11:50:02 PDT
Comment on attachment 608039 [details] [diff] [review]
Cache temporary surfaces used during canvas compositing

Thanks Jeff, useful patch.
Comment 11 Jeff Gilbert [:jgilbert] 2012-03-21 12:55:17 PDT
Running try before pushing:
https://tbpl.mozilla.org/?tree=Try&rev=5a628dc158a2
Comment 12 Jeff Gilbert [:jgilbert] 2012-03-21 14:55:01 PDT
Comment on attachment 608039 [details] [diff] [review]
Cache temporary surfaces used during canvas compositing

Memory management issues in BasicLayers and OGLLayers.
Comment 13 Jeff Gilbert [:jgilbert] 2012-03-21 19:50:17 PDT
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
Comment 14 Benoit Girard (:BenWa) 2012-03-22 06:23:19 PDT
I see more leaks in your try push. Why is mCachedTempSurface no longer a nsRefPtr?
Comment 15 Jeff Gilbert [:jgilbert] 2012-03-22 10:18:23 PDT
(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.
Comment 16 Jeff Gilbert [:jgilbert] 2012-03-22 10:21:43 PDT
Created attachment 608375 [details] [diff] [review]
Cache temporary surfaces used during canvas compositing

https://tbpl.mozilla.org/?tree=Try&rev=865f7f3674eb
Comment 18 Jeff Gilbert [:jgilbert] 2012-03-22 19:13:13 PDT
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.
Comment 19 Kartikaya Gupta (email:kats@mozilla.com) 2012-03-22 22:01:07 PDT
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.
Comment 20 Kartikaya Gupta (email:kats@mozilla.com) 2012-03-23 06:37:36 PDT
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.
Comment 21 Jeff Gilbert [:jgilbert] 2012-03-23 14:32:01 PDT
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+.
Comment 23 Matt Brubeck (:mbrubeck) 2012-03-26 11:26:32 PDT
https://hg.mozilla.org/mozilla-central/rev/5574b6eadb13

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