987828 - WebGL backend does FakeVertexAttrib0-machinery when targeting pure GLES2 devices, but it shouln't need to.

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Jukka Jylänki]

Looking at a profile of UE3 on Android, the single biggest time-consumer is the WebGL DrawElements function, which in turn calls into the following signatures:

mozilla::WebGLContext::UndoFakeVertexAttrib0
mozilla::WebGLContext::WhatDoesVertexAttrib0Need

These don't take up much time, but as it looks like there are lot of these small spenders in WebGL DrawElements, these do accumulate up to add overhead compared to native. Talking with bjacob, he suggested adding a bug to remind about optimizing this machinery out on the devices that are pure GLES2, where there is no strict need to bind something to attribute 0 like there is for desktop GL.

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Move draw-calls-stuff to a new WebGLContextDraw.cpp file, optimize attrib0 stuff by #ifdef ANDROID and MOZ_LIKELY

So on the profiles that we were looking at with Jukka, in addition to attrib0 stuff, there also was fakeblack stuff. Since the two are similar, they are probably OK to try to address together in 1 patch.

The bulk of this patch is moving draw-calls stuff from WebGLContextVertices.cpp to a new WebGLContextDraw.cpp file, and then, moving the attrib0 and fakeblack code, that was mostly only used by draw-calls code, to the same file (from WebGLContextGL.cpp).

The next thing is, in WhatDoesAttrib0Need(), we #ifdef ANDROID a fast exit path, and in the Do/Undo Attrib0 functions that call it, the fast exit path is now MOZ_LIKELY. All together, being in the same file allowing inlining (even on B2G where we don't have unified builds at the moment), the attrib0 stuff should be zero overhead on Android/B2G.

Regarding the fakeblack stuff, it had already MOZ_LIKELY exit paths, and it can't be #ifdef'd away, but it should also benefit from being all in the same file, and if we still have profiles telling us we should optimize more, then we can start looking at more changes such as isolating fast-exit-path code into smaller inline functions to get better inlining. But maybe that won't be needed.

Comment 2

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8397484 [details] [diff] [review]
Move draw-calls-stuff to a new WebGLContextDraw.cpp file, optimize attrib0 stuff by #ifdef ANDROID and MOZ_LIKELY

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

I'm assuming very little code was changed, and most of it was moved. It's really hard for me to know, though. Try to do the changes and moves in different patches, please!

::: content/canvas/src/WebGLContextDraw.cpp
@@ +448,5 @@
> +WebGLContext::WhatDoesVertexAttrib0Need()
> +{
> +    // Android and B2G never need any of this because they are ES-only
> +#ifdef ANDROID
> +    return WebGLVertexAttrib0Status::Default;

There's not a great reason to special-case this, since we should get the same thing from the GLES2 check below.

@@ +451,5 @@
> +#ifdef ANDROID
> +    return WebGLVertexAttrib0Status::Default;
> +#endif
> +
> +    // here we may assume that mCurrentProgram != null

Let's add an assert for this.

Comment 3

[Kelsey Gilbert [:jgilbert]]

R- for the #ifdef. Other comments are nits.

Comment 4

[Jukka Jylänki]

Is ANDROID defined for B2G builds too? If not, can we expand the ifdefs to apply for B2G path as well, since they are similar? If there is a GLES2 ifdef, then perhaps that would be better, i.e. to use

#ifdef GLES2
...
#endif

instead of 

#if defined(ANDROID) || defined(B2G)
...
#endif

since the common denominator is that on platforms where we know at compile-time that we are building for GLES2, the fakevertexattrib0-machinery won't be needed?

Comment 5

[Kelsey Gilbert [:jgilbert]]

(In reply to Jukka Jylänki from comment #4)
> Is ANDROID defined for B2G builds too? If not, can we expand the ifdefs to
> apply for B2G path as well, since they are similar? If there is a GLES2
> ifdef, then perhaps that would be better, i.e. to use
> 
> #ifdef GLES2
> ...
> #endif
> 
> instead of 
> 
> #if defined(ANDROID) || defined(B2G)
> ...
> #endif
> 
> since the common denominator is that on platforms where we know at
> compile-time that we are building for GLES2, the fakevertexattrib0-machinery
> won't be needed?

It's so small that we shouldn't bother not building it. Just do it programatically.

Comment 6

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Jeff Gilbert [:jgilbert] from comment #2)
> Comment on attachment 8397484 [details] [diff] [review]
> Move draw-calls-stuff to a new WebGLContextDraw.cpp file, optimize attrib0
> stuff by #ifdef ANDROID and MOZ_LIKELY
> 
> Review of attachment 8397484 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm assuming very little code was changed, and most of it was moved. It's
> really hard for me to know, though. Try to do the changes and moves in
> different patches, please!

Sorry about that. I've remade the patch so it's now well separated. Attaching below.

> 
> ::: content/canvas/src/WebGLContextDraw.cpp
> @@ +448,5 @@
> > +WebGLContext::WhatDoesVertexAttrib0Need()
> > +{
> > +    // Android and B2G never need any of this because they are ES-only
> > +#ifdef ANDROID
> > +    return WebGLVertexAttrib0Status::Default;
> 
> There's not a great reason to special-case this, since we should get the
> same thing from the GLES2 check below.

The problem is that IsGLES2() actually showed up on Jukka's profile. Not hugely, but then, Jukka's profile was a long tail of non-huge things, which add up to enough for us to try optimizing, and IsGLES2() was one of them.

But, there is a way out: if instead of IsGLES2() we call the simpler IsGLES(), which is a single integer comparison, and enclose it in a MOZ_LIKELY(), then this should become very cheap.

Thinking more about it, IsGLES() already means "GL ES 2 or newer" since we don't support GL ES 1, so IsGLES2() is useless. So I'll remove it in patches below.

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Move stuff to a new WebGLContextDraw.cpp file

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Optimize attrib0 stuff

Comment 9

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Use IsGLES everywhere instead of IsGLES2

Comment 10

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Remove IsGLES2

Comment 11

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8398706 [details] [diff] [review]
Move stuff to a new WebGLContextDraw.cpp file

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

Awesome, thanks for splitting this up like this.

Comment 12

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8398708 [details] [diff] [review]
Optimize attrib0 stuff

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

::: content/canvas/src/WebGLContextDraw.cpp
@@ +464,2 @@
>           : mCurrentProgram->IsAttribInUse(0) ? WebGLVertexAttrib0Status::EmulatedInitializedArray
>                                               : WebGLVertexAttrib0Status::EmulatedUninitializedArray;

This is increasingly gross. This should really be:
if (MOZ_LIKELY(gl->IsGLES2() ||
               mBoundVertexArray->IsAttribArrayEnabled(0)))
{
  return Default
}

return IsAttribInUse ? Init
                     : Uninit

Comment 13

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8398710 [details] [diff] [review]
Use IsGLES everywhere instead of IsGLES2

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

Great change!

Comment 14

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8398711 [details] [diff] [review]
Remove IsGLES2

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

\o/ Death to code!

Comment 15

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

(We should be able to just have IsGLES unconditionally return true on ANDROID || GONK, and then add an assert in the constructor that mProfile == ContextProfile::OpenGLES with the same ifdef, in case we forget and end up supporting non-GLES on mobile.)

Comment 16

[Benoit Jacob [:bjacob] (mostly away)]

Yes, we could do that; but that might also not matter: with the patches here, the check should inline into

  if (MOZ_LIKELY(mProfile == ContextProfile::OpenGLES)) {

Which costs only one integer comparison and no jump... so that should be totally negligible already. A big improvement over the situation where we call the more complex IsGLES2(), which does not get inlined and shows up on profiles.

Comment 17

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Jukka Jylänki from comment #4)
> Is ANDROID defined for B2G builds too?

Yes, it is. Don't worry, I'm not leaving B2G out :-)

Comment 18

[Benoit Jacob [:bjacob] (mostly away)]

(Well, again, that doesn't matter anymore, as the current patches don't test for ANDROID).

Comment 19

[Benoit Jacob [:bjacob] (mostly away)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a740600cf950
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e557ecd55785
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/bdd4e53bd7ce
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7d6070186810

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a740600cf950
https://hg.mozilla.org/mozilla-central/rev/e557ecd55785
https://hg.mozilla.org/mozilla-central/rev/bdd4e53bd7ce
https://hg.mozilla.org/mozilla-central/rev/7d6070186810