996269 - Lazily clear the backbuffer

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Kelsey Gilbert [:jgilbert]]

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.

Comment 1

[Kelsey Gilbert [:jgilbert]]

Attachment: jit-clear

Comment 2

[u480271]

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? :-)

Comment 3

[Kelsey Gilbert [:jgilbert]]

https://tbpl.mozilla.org/?tree=Try&rev=07bc3ac10110

Comment 4

[Kelsey Gilbert [:jgilbert]]

(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. :)

Comment 5

[Kelsey Gilbert [:jgilbert]]

Attachment: jit-clear-2

:mattwoodrow just killed Render, so this needed a fairly obvious change.
r+ still from :kamidphish.

Comment 6

[Kelsey Gilbert [:jgilbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f323de6b9186

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f323de6b9186

Comment 8

[Jeff Muizelaar [:jrmuizel]]

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?

Comment 9

[Kelsey Gilbert [:jgilbert]]

(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?