Last Comment Bug 638323 - Cache the current GLContext to avoid calling MakeCurrent unnecessarily
: Cache the current GLContext to avoid calling MakeCurrent unnecessarily
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Canvas: WebGL (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: Matt Woodrow (:mattwoodrow)
:
Mentors:
Depends on:
Blocks: 638321
  Show dependency treegraph
 
Reported: 2011-03-02 18:30 PST by Matt Woodrow (:mattwoodrow)
Modified: 2011-04-18 16:12 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Skip unneeded MakeCurrent calls. (2.12 KB, patch)
2011-03-02 18:30 PST, Matt Woodrow (:mattwoodrow)
jacob.benoit.1: review-
Details | Diff | Review
Skip unneeded MakeCurrent calls. v2 (703 bytes, patch)
2011-04-14 11:58 PDT, Matt Woodrow (:mattwoodrow)
jacob.benoit.1: review+
Details | Diff | Review
Skip unneeded MakeCurrent calls. v3 (710 bytes, patch)
2011-04-18 15:44 PDT, Matt Woodrow (:mattwoodrow)
matt.woodrow: review+
Details | Diff | Review

Description Matt Woodrow (:mattwoodrow) 2011-03-02 18:30:38 PST
Created attachment 516477 [details] [diff] [review]
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) 2011-03-03 04:51:37 PST
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) 2011-03-03 04:53:47 PST
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) 2011-04-14 11:58:40 PDT
Created attachment 526065 [details] [diff] [review]
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) 2011-04-18 10:52:13 PDT
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) 2011-04-18 13:47:27 PDT
> 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) 2011-04-18 14:10:09 PDT
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) 2011-04-18 15:44:08 PDT
Created attachment 526854 [details] [diff] [review]
Skip unneeded MakeCurrent calls. v3

Fixed compile error. Carrying forward r=bjacob
Comment 8 Matt Woodrow (:mattwoodrow) 2011-04-18 16:12:29 PDT
http://hg.mozilla.org/mozilla-central/rev/20725bef512f

Note You need to log in before you can comment on or make changes to this bug.