963549 - Skip the glClear in CompositorOGL::BeginFrame when we don't need it

Status: RESOLVED WONTFIX

(Core :: Graphics, defect)

Description

[Markus Stange [:mstange]]

Bug 956401 inadvertently disabled the clear call at http://hg.mozilla.org/mozilla-central/annotate/9d650c07b547/gfx/layers/opengl/CompositorOGL.cpp#l869 which resulted a TART improvement on our OS X Talos machines of 5-7%. See bug 961229 for details.

When we know that our painting covers the whole window, the clear call is useless and should be skipped.

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

For tiled mobile GPUs, this is potentially a bad idea -- such a clear is usually implemented very efficiently (just setting a single value, instead of actually clearing every pixel), and is often pretty key for performance.  Binding a surface to draw (whether FBO, EGLSurface, whatever) and then clearing it is a sign to the driver that you don't care about what was there before.  Without the clear, the GPU has to load the old memory into the render tile just to draw over it.

We should measure perf on mobile, especially Firefox OS, before committing to this universally.. it is almost certainly a good idea on desktop, though.

Comment 2

[Andrew Comminos [:acomminos]]

So I've written up a patch that disables the scissor test in the GL compositor wherever it's not being immediately used, which makes the clear at the end of CompositorOGL::BeginFrame actually unbounded.

Perfherder results would suggest that, as one would sanely expect, our GL implementation on OS X is doing the right thing (lazily clearing):

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=732b79f1e57f&newProject=try&newRevision=b99752e6d2c6&framework=1&filter=osx-10-10&showOnlyImportant=0

I suspect that in prior testing, we had a scissor rect left over from the end of a previous frame, causing bounded (slower) clears to be performed.

Comment 3

[Markus Stange [:mstange]]

Good!

Comment 4

[Andrew Comminos [:acomminos]]

Going to close this for now- if a reason arises that we *shouldn't* perform an unbounded clear, I'll reopen.