Closed Bug 1378571 Opened 2 years ago Closed 2 years ago

Avoid unnecessary MakeCurrentImpl calls when GL context is already current

Categories

(Core :: Canvas: WebGL, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: svargas, Assigned: svargas)

Details

Attachments

(1 file, 5 obsolete files)

No description provided.
Assignee: nobody → svargas
Attachment #8883763 - Flags: review?(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
Attachment #8883763 - Flags: review?(jgilbert) → review-
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.
Attachment #8883763 - Attachment is obsolete: true
Attachment #8884094 - Flags: review?(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;
Attachment #8884094 - Flags: review?(jgilbert) → review-
Attachment #8884094 - Attachment is obsolete: true
Attachment #8884111 - Flags: review?(jgilbert)
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.
Attachment #8884111 - Attachment is obsolete: true
Attachment #8884111 - Flags: review?(jgilbert)
Attachment #8884124 - Flags: review?(jgilbert)
Modify ContextProviderEGL to use 'UseTLSIsCurrent' if 'sEGLLibrary.IsANGLE()' is true. Also pushed to TryServer:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7693f2800644f068d11d3bd5aeac6382ca4ddc64
Attachment #8884124 - Attachment is obsolete: true
Attachment #8884124 - Flags: review?(jgilbert)
Attachment #8884127 - Flags: review?(jgilbert)
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
Attachment #8884127 - Attachment is obsolete: true
Attachment #8884127 - Flags: review?(jgilbert)
Attachment #8884460 - Flags: review?(jgilbert)
Attachment #8884460 - Flags: review?(jgilbert) → review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/122e0e90b9a1
Avoid unnecessary MakeCurrentImpl calls when GL context is already current - r=jgilbert
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/122e0e90b9a1
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.