1378571 - Avoid unnecessary MakeCurrentImpl calls when GL context is already current

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, enhancement)

Comment 1

[Samuel Vargas [:svargas]]

Attachment: 0001-Bug-1378571-Avoid-unnecessary-MakeCurrentImpl-calls-.patch

Comment 2

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8883763 [details] [diff] [review]
0001-Bug-1378571-Avoid-unnecessary-MakeCurrentImpl-calls-.patch

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

Good start!

There should also be a flag on the context that mediates whether the context can take advantage of this optimization. This should be a const bool member.

::: gfx/gl/GLContext.cpp
@@ +285,4 @@
>      mMaxViewportDims[0] = 0;
>      mMaxViewportDims[1] = 0;
>      mOwningThreadId = PlatformThread::CurrentId();
> +    MOZ_ALWAYS_TRUE(GLContext::currentContext.init());

With code inside a MOZ_ALWAYS_TRUE, prefer to put spaces around it, like:
MOZ_ALWAYS_TRUE( sCurrentContext.init() );

It's probably worth following this with a `sCurrentContext.set(nullptr)`.

@@ +3003,5 @@
>  
> +bool GLContext::MakeCurrent(bool aForce) {
> +    if (IsDestroyed()) {
> +        return false;
> +    }

The style should be:
bool
GLContext::MakeCurrent(bool aForce)
{
    if (IsDestroyed())
        return false;

#ifdef MOZ_GL_DEBUG
    PR_SetThreadPrivate...

@@ +3020,5 @@
> +    #endif
> +    #endif
> +    if (GLContext::currentContext.get() == this) {
> +        return true;
> +    }

if (sCurrentContext.get() == this)
    return true;

@@ +3024,5 @@
> +    }
> +
> +    bool success = MakeCurrentImpl(aForce);
> +    if (success) {
> +        GLContext::currentContext.set(this);

GLContext:: is implicit

::: gfx/gl/GLContext.h
@@ +197,4 @@
>  {
>  public:
>      MOZ_DECLARE_WEAKREFERENCE_TYPENAME(GLContext)
> +    static MOZ_THREAD_LOCAL(GLContext*) currentContext;

sCurrentContext

Comment 3

[Samuel Vargas [:svargas]]

Attachment: 0001-Bug-1378571-Avoid-unnecessary-MakeCurrentImpl-calls-.patch

Implemented fixes, added bool canUseTLSIsCurrent = false to GLContext constructor.

Need to attach another patch for toggling it on / off depending on the context provider still.

Comment 4

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8884094 [details] [diff] [review]
0001-Bug-1378571-Avoid-unnecessary-MakeCurrentImpl-calls-.patch

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

::: gfx/gl/GLContext.cpp
@@ +3007,5 @@
> +{
> +    if (IsDestroyed())
> +        return false;
> +
> +    #ifdef MOZ_GL_DEBUG

This #ifdef here should not be indented with the scope, or its contents must be indented with a new scope level.

@@ +3022,5 @@
> +                        "MakeCurrent() called on different thread than this context was created on!");
> +    #endif
> +    #endif
> +    if (mCanUseTLSIsCurrent && sCurrentContext.get() == this)
> +        return true;

Add `MOZ_ASSERT(IsCurrent())` before the `return true` here.

@@ +3028,5 @@
> +    bool success = MakeCurrentImpl(aForce);
> +    if (mCanUseTLSIsCurrent && success) {
> +        sCurrentContext.set(this);
> +    }
> +    return success;

I would write this as:
if (!MakeCurrentImpl(aForce))
  return false;
if (mCanUseTLSIsCurrent) {
  sCurrentContext.set(this);
}
return true;

Comment 5

[Samuel Vargas [:svargas]]

Attachment: 0001-Bug-1378571-Avoid-unnecessary-MakeCurrentImpl-calls-.patch

Comment 6

[Samuel Vargas [:svargas]]

Attachment: 0001-Bug-1378571-Avoid-unnecessary-MakeCurrentImpl-calls-.patch

Modified the patch to use a 'gl.use-tls-is-current' key that either force enables, force disables, or uses the default useTLSISCurrent value passed into the GLContext constructor by ContextProviders.

Comment 7

[Samuel Vargas [:svargas]]

Attachment: 0001-Bug-1378571-Avoid-unnecessary-MakeCurrentImpl-calls-.patch

Modify ContextProviderEGL to use 'UseTLSIsCurrent' if 'sEGLLibrary.IsANGLE()' is true. Also pushed to TryServer:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7693f2800644f068d11d3bd5aeac6382ca4ddc64

Comment 8

[Samuel Vargas [:svargas]]

Attachment: 0001-Bug-1378571-Avoid-unnecessary-MakeCurrentImpl-calls-.patch

Two new changes:

1) Switch from 'Once' to 'Live' in gfxPrefs.h for faster future debugging with the gl.use-tls-is-current preference.

2) Add '&& !aForce' to GLContext::MakeCurrent to allow users to explicitly bypass the mUseTLSIsCurrent behavior (was causing failures on misc WebGL conformance tests without this).

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1d61d1fc3bf57e79c222833b2da4e1f64f526f4

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/122e0e90b9a1
Avoid unnecessary MakeCurrentImpl calls when GL context is already current - r=jgilbert

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/122e0e90b9a1