Closed Bug 996269 Opened 5 years ago Closed 5 years ago

Lazily clear the backbuffer

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: jgilbert, Assigned: jgilbert)

Details

Attachments

(2 files)

We should not eagerly clear the backbuffer, since we'll generally waste a huge amount of memory bandwidth rasterizing the clear we do to the persistent FB data, when it's likely cleared later in the frame anyways.
Attached patch jit-clearSplinter Review
Attachment #8406466 - Flags: review?(dglastonbury)
Comment on attachment 8406466 [details] [diff] [review]
jit-clear

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

::: content/canvas/src/WebGLContext.cpp
@@ +793,5 @@
>      gl->fClearColor(0.0f, 0.0f, 0.0f, 0.0f);
>      gl->fClearDepth(1.0f);
>      gl->fClearStencil(0);
>  
> +    //gl->ClearSafely();

Remove or comment

@@ +818,5 @@
> +
> +#ifdef DEBUG
> +    gl->MakeCurrent();
> +
> +    GLuint fb = 0;

Should this be some other value (0xFFFFFFFF) so we know that GetIntegerv wrote the value 0?

::: content/canvas/src/WebGLContextDraw.cpp
@@ +266,5 @@
>              ErrorInvalidFramebufferOperation("%s: incomplete framebuffer", info);
>              return false;
>          }
> +    } else {
> +        ClearBackbufferIfNeeded();ClearBackbufferIfNeeded();

"Scratch is so nice gotta hear it twice" (https://www.youtube.com/watch?v=d1RaOSuPIkk#t=30)

::: content/canvas/src/WebGLContextFramebufferOperations.cpp
@@ +40,5 @@
>      }
>  
>      // Ok, we're clearing the default framebuffer/screen.
>  
> +    gl->fClear(mask);

Where did all the code go? :-)
Attachment #8406466 - Flags: review?(dglastonbury) → review+
(In reply to Dan Glastonbury :djg :kamidphish from comment #2)
> Comment on attachment 8406466 [details] [diff] [review]
> jit-clear
> 
> Review of attachment 8406466 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/src/WebGLContext.cpp
> @@ +793,5 @@
> >      gl->fClearColor(0.0f, 0.0f, 0.0f, 0.0f);
> >      gl->fClearDepth(1.0f);
> >      gl->fClearStencil(0);
> >  
> > +    //gl->ClearSafely();
> 
> Remove or comment
removed
> 
> @@ +818,5 @@
> > +
> > +#ifdef DEBUG
> > +    gl->MakeCurrent();
> > +
> > +    GLuint fb = 0;
> 
> Should this be some other value (0xFFFFFFFF) so we know that GetIntegerv
> wrote the value 0?
I think it's only worth doing this if it's reasonable that there'll be an error. I don't think that's likely here.
> 
> ::: content/canvas/src/WebGLContextDraw.cpp
> @@ +266,5 @@
> >              ErrorInvalidFramebufferOperation("%s: incomplete framebuffer", info);
> >              return false;
> >          }
> > +    } else {
> > +        ClearBackbufferIfNeeded();ClearBackbufferIfNeeded();
> 
> "Scratch is so nice gotta hear it twice"
> (https://www.youtube.com/watch?v=d1RaOSuPIkk#t=30)
oops!
> 
> ::: content/canvas/src/WebGLContextFramebufferOperations.cpp
> @@ +40,5 @@
> >      }
> >  
> >      // Ok, we're clearing the default framebuffer/screen.
> >  
> > +    gl->fClear(mask);
> 
> Where did all the code go? :-)
The state queries we have to do here to make it worth it not to do the 'extra' clear are probably not worth it. WebGL devs should just not clear unnecessarily. :)
Attached patch jit-clear-2Splinter Review
:mattwoodrow just killed Render, so this needed a fairly obvious change.
r+ still from :kamidphish.
Attachment #8408509 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/f323de6b9186
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
We're seeing a bunch of time being spent in glClear and it looks like the patch might be the culprit. Did this patch show any performance improvement?
(In reply to Jeff Muizelaar [:jrmuizel] from comment #8)
> We're seeing a bunch of time being spent in glClear and it looks like the
> patch might be the culprit. Did this patch show any performance improvement?

I don't remember if I tested it, but architecturally, this should be better for perf.

The change which could cause worse perf is that we removed the logic for skipping a clear if the clear would be redundant with the already-cleared buffer. This is an optimization that should happen at the app level. (don't redundantly clear) Maybe an app redundantly clears?
You need to log in before you can comment on or make changes to this bug.