Cache the current GLContext to avoid calling MakeCurrent unnecessarily

RESOLVED FIXED

Status

()

Core
Canvas: WebGL
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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).
Attachment #516477 - Flags: review?(bjacob)
(Assignee)

Updated

6 years ago
Blocks: 638321
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.
Attachment #516477 - Flags: review?(bjacob) → review-
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).
(Assignee)

Comment 3

6 years ago
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.
Assignee: nobody → matt.woodrow+bugzilla
Attachment #516477 - Attachment is obsolete: true
Attachment #526065 - Flags: review?(bjacob)
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.
(Assignee)

Comment 5

6 years ago
> 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 on attachment 526065 [details] [diff] [review]
Skip unneeded MakeCurrent calls. v2

Oh! OK. Sorry, I don't know Objective C++.
Attachment #526065 - Flags: review?(bjacob) → review+
(Assignee)

Comment 7

6 years ago
Created attachment 526854 [details] [diff] [review]
Skip unneeded MakeCurrent calls. v3

Fixed compile error. Carrying forward r=bjacob
Attachment #526854 - Flags: review+
(Assignee)

Comment 8

6 years ago
http://hg.mozilla.org/mozilla-central/rev/20725bef512f
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.