638323 - Cache the current GLContext to avoid calling MakeCurrent unnecessarily

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Matt Woodrow (:mattwoodrow)]

Attachment: Skip unneeded MakeCurrent calls.

GLContext::MakeCurrent shows up quite considerably when profiling the WebGL component of jsgamebench.

Under a call to nsIDOMWebGLRenderingContext_Uniform3f (11.5%), 2.7% is spent in MakeCurrent, and only 3.9% inside glUniform3f(). The remaining time appears to be XPConnect overhead, will file a bug for this separately.

A simple patch to make sCurrentGLContext available in release mode, and making MakeCurrent() a nop if this matches takes me benchmark score from 1400 -> 1950.

I'm not sure if there's any issue with this other than the possiblity of GL being on another thread (mentioned in the current comments, not happening in our code currently) and other code accessing gl without going through GLContext (again, not currently an issue).

Comment 1

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

Comment on attachment 516477 [details] [diff] [review]
Skip unneeded MakeCurrent calls.

This is already checked in certain back-ends, at least in EGL and GLX, but not in others, such as CGL.

I'm all for your change, doing it once and for all in GLContext, but this needs to be done together with removing the back-ends-specific code.

Comment 2

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

I don't have any issue with this as, as you say, we currently don't make GL calls from non-main threads.

If we did, we'd have to make sCurrentContext a TLS, and then reading it would be more expensive. It would then potentially become a bad idea performance-wise, as certain back-ends are already doing exactly that internally (e.g. NVIDIA driver on GLX).

Comment 3

[Matt Woodrow (:mattwoodrow)]

Attachment: Skip unneeded MakeCurrent calls. v2

Looks like most platforms already implement this sort of check with TLS support.

We might as well continue using these and just start doing the same on mac.

Comment 4

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

Other platforms use platform-specific calls to get the current context, e.g. glXGetCurrentContext. CGL has a similar function: CGLGetCurrentContext.

I would suggest that we try to stay homogeneous across platforms:
 * either make CGL use CGLGetCurrentContext
 * or decide that a global non-TLS variable is enough (because we only do OpenGL calls from 1 thread) and therefore switch all GL context providers to using such a non-TLS global variable and drop GetCurrentContext calls everywhere.

Comment 5

[Matt Woodrow (:mattwoodrow)]

> I would suggest that we try to stay homogeneous across platforms:
>  * either make CGL use CGLGetCurrentContext

This is almost what I'm doing! Except I need to qref the extra chunk from bug 586508.

Unlike what the filename would suggest, we don't actually use CGL. We are using NSOpenGL instead.

Comment 6

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

Comment on attachment 526065 [details] [diff] [review]
Skip unneeded MakeCurrent calls. v2

Oh! OK. Sorry, I don't know Objective C++.

Comment 7

[Matt Woodrow (:mattwoodrow)]

Attachment: Skip unneeded MakeCurrent calls. v3

Fixed compile error. Carrying forward r=bjacob

Comment 8

[Matt Woodrow (:mattwoodrow)]

http://hg.mozilla.org/mozilla-central/rev/20725bef512f