Bug 156689

Summary: Creating a large number of WebGL contexts should recycle older contexts
Product: WebKit Reporter: Antoine Quint <graouts>
Component: WebGLAssignee: Antoine Quint <graouts>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, cdumez, commit-queue, dino, esprehn+autocc, graouts, gyuyoung.kim, kondapallykalyan, noam, ryanhaddad, sabouhallawa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Safari 9   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing none

Description Antoine Quint 2016-04-18 02:13:34 PDT
We currently stop creating WebGL contexts once a maximum of 64 WebGL contexts have been created on a page. Other browsers have a limit of 16 concurrent active WebGL contexts and they lose older contexts when the developer creates a new context, logging a warning to the console. We should follow a similar approach.
Comment 1 Antoine Quint 2016-04-18 02:13:50 PDT
<rdar://problem/19535330>
Comment 2 Radar WebKit Bug Importer 2016-04-18 02:14:23 PDT
<rdar://problem/25774766>
Comment 3 Antoine Quint 2016-04-18 02:15:47 PDT
<rdar://problem/19535330>
Comment 4 Antoine Quint 2016-04-18 02:28:40 PDT
Created attachment 276625 [details]
Patch
Comment 5 Dean Jackson 2016-04-18 12:12:42 PDT
Bit of a worry that mac-debug EWS bombed out.
38815 tests ran as expected, 2671 didn't

You should probably upload a patch before landing to see if that happens again, and is happening all the time on other patches.
Comment 6 Dean Jackson 2016-04-18 12:22:52 PDT
Comment on attachment 276625 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=276625&action=review

> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:3780
> +    // Using SyntheticLostContext means the developer won't be able to force the restoration
> +    // of the context by calling preventDefault() in a "webglcontextlost" event handler.

I wasn't aware we did that on preventDefault for real lost contexts. That sounds like terrible behaviour.

> Source/WebCore/platform/graphics/GraphicsContext3D.h:1269
> +    static Vector<GraphicsContext3D*>* s_activeContexts;

Is this exposed to other platforms? I guess they never used it since otherwise you'd have to change their .cpp files too.

I wonder if we should do this for all ports.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:1860
> +void GraphicsContext3D::recycleContext()
> +{
> +#if ENABLE(WEBGL)
> +    if (m_webglContext)
> +        m_webglContext->recycleContext();
> +#endif
> +}

I'm not sure we ever have a GC3D in a situation where ENABLE(WEBGL) is false.
Comment 7 Said Abou-Hallawa 2016-04-18 13:16:26 PDT
Comment on attachment 276625 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=276625&action=review

> Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:67
> +const int maxActiveContexts = 16;

This constant can be a member the class and can be moved to the header file also.

private:
    static const int MaxActiveContexts = 16;

> Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:68
> +Vector<GraphicsContext3D*>* GraphicsContext3D::s_activeContexts;

Why do not we use NeverDestroyed template for s_activeContexts? activeContexts does not even need to be a member of GraphicsContext3D class.

static Vector<GraphicsContext3D*>& activeContexts()
{
    static NeverDestroyed<Vector<GraphicsContext3D*>> s_activeContexts;
    return s_activeContexts;
}

You can also get rid of the allocation in the create() and the assertion in the destructor.

> Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:137
>      return context.release();

I think we can use return WTFMove(context) instead. Also the type of the return value can be change to RefPtr.
Comment 8 Antoine Quint 2016-04-18 14:05:37 PDT
Created attachment 276665 [details]
Patch
Comment 9 Antoine Quint 2016-04-18 14:20:17 PDT
Created attachment 276668 [details]
Patch
Comment 10 Antoine Quint 2016-04-18 14:30:07 PDT
Created attachment 276670 [details]
Patch
Comment 11 Said Abou-Hallawa 2016-04-18 15:26:37 PDT
Looks good to me. Unofficial r=me.
Comment 12 Alexey Proskuryakov 2016-04-18 21:03:56 PDT
Debug EWS failures look strange, but possibly related to this patch. I just rebooted the bot that was seeing them to be more sure about what's going on.
Comment 13 Antoine Quint 2016-04-19 00:11:46 PDT
(In reply to comment #12)
> Debug EWS failures look strange, but possibly related to this patch. I just
> rebooted the bot that was seeing them to be more sure about what's going on.

It seems that since rebooting the bots are unable to build at all. FWIW, I can't reproduce the WebGL test timeouts when running the tests locally in debug.
Comment 14 Antoine Quint 2016-04-19 03:21:41 PDT
Created attachment 276711 [details]
Patch
Comment 15 Antoine Quint 2016-04-19 03:27:46 PDT
Actually, I did see some crashes due to the ASSERT(activeContexts().contains(this)) I had added in the deconstructor which was a mistake since contexts can be removed from the list of active contexts prior to being GC'd.
Comment 16 Alexey Proskuryakov 2016-04-19 10:24:46 PDT
I filed bug 156750 to track debug EWS problems.

Maybe there was a change recently that makes WindowServer freeze when running WebKit tests. Or maybe it's this patch that kills them one by one.
Comment 17 Alexey Proskuryakov 2016-04-19 10:27:42 PDT
Comment on attachment 276711 [details]
Patch

Marking r- to get this patch out of EWS queue.

Antoine, could you please work with Ryan Haddad to have this patch tested on Mac Pros manually?
Comment 18 Antoine Quint 2016-04-20 06:56:33 PDT
Created attachment 276821 [details]
Patch
Comment 19 Antoine Quint 2016-04-20 07:00:42 PDT
I think I finally found the cause of the debug crasher. In recycleContext(), we needed to call destroyGraphicsContext3D() after calling forceLostContext(SyntheticLostContext) so as to disassociate the WebGL context and the GraphicsContext3D objects and clear the Open GL context. All WebGL tests now pass for me in debug builds without a crash or timeout. I hope this will keep the mac-debug bot happy this time.
Comment 20 Antoine Quint 2016-04-20 07:37:37 PDT
The ios-sim test failures are not related to this patch.
Comment 21 Said Abou-Hallawa 2016-04-20 10:48:18 PDT
Comment on attachment 276821 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=276821&action=review

Looks good to me. Unofficial r=me.

> Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:136
> +        contexts.removeFirst(contextToLose);

GraphicsContext3D::recycleContext() calls WebGLRenderingContextBase::recycleContext()
    WebGLRenderingContextBase::recycleContext() calls WebGLRenderingContextBase::destroyGraphicsContext3D()
        WebGLRenderingContextBase::destroyGraphicsContext3D() sets m_context = nullptr
                Setting m_context = nullptr may call GraphicsContext3D()::~GraphicsContext3D() if it is the last reference.
You added activeContexts().removeFirst(this) to GraphicsContext3D()::~GraphicsContext3D(). So I think there is no need to call contexts.removeFirst(contextToLose) here.

Actually I think it is safer and cleaner to call activeContexts().removeFirst() only from the GraphicsContext3D()::~GraphicsContext3D() because of the following reasons:

1. In the destructor of GraphicsContext3D(), we are sure that "this" is still a valid pointer but after that it will no longer be valid. So this is exactly the right time to remove the raw pointer from the activeContexts().
2. This function returns a RefPtr<GraphicsContext3D> which may be ref-counted multiple times.  When calling recycleContext(), for a previously allocated PtrRef<GraphicsContext3D> by this function, might not actually dispose its raw pointer. I am not sure if this can happen or not. But if it happens, that means there will be extra contexts hanging around but not recorded by activeContexts() because we removed their references from the activeContexts() even though their raw pointer are still alive.

> Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:139
> +    contexts.append(context.get());

If you decide to remove the  contexts.removeFirst(contextToLose), then you will need to check:
if (contexts.size() >= MaxActiveContexts)
    return nullptr;

before this append(). You may also want to move the code which tries to find space in activeContexts() before creating the the GraphicsContext3D.

if (contexts.size() >= MaxActiveContexts) {
    ...
}

if (contexts.size() >= MaxActiveContexts)
   return nullptr;

RefPtr<GraphicsContext3D> context = adoptRef(new GraphicsContext3D(attrs, hostWindow, renderStyle));

> Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:349
> +    activeContexts().removeFirst(this);

I think if you have removing the raw pointer from the activeContexts() only in the destructor, then you can safely asserts that activeContexts().contains(this).

> Source/WebCore/platform/graphics/win/GraphicsContext3DWin.cpp:63
>          return 0;

0 ==> nullptr.
Comment 22 Alexey Proskuryakov 2016-04-20 11:01:24 PDT
Comment on attachment 276821 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=276821&action=review

> Source/WebCore/platform/graphics/cairo/GraphicsContext3DCairo.cpp:77
> +    return WTFMove(context);

cq-, as this should be "return context".
Comment 23 Antoine Quint 2016-04-20 11:07:12 PDT
(In reply to comment #21)
> Comment on attachment 276821 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=276821&action=review
> 
> Looks good to me. Unofficial r=me.
> 
> > Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:136
> > +        contexts.removeFirst(contextToLose);
> 
> GraphicsContext3D::recycleContext() calls
> WebGLRenderingContextBase::recycleContext()
>     WebGLRenderingContextBase::recycleContext() calls
> WebGLRenderingContextBase::destroyGraphicsContext3D()
>         WebGLRenderingContextBase::destroyGraphicsContext3D() sets m_context
> = nullptr
>                 Setting m_context = nullptr may call
> GraphicsContext3D()::~GraphicsContext3D() if it is the last reference.
> You added activeContexts().removeFirst(this) to
> GraphicsContext3D()::~GraphicsContext3D(). So I think there is no need to
> call contexts.removeFirst(contextToLose) here.
> 
> Actually I think it is safer and cleaner to call
> activeContexts().removeFirst() only from the
> GraphicsContext3D()::~GraphicsContext3D() because of the following reasons:
> 
> 1. In the destructor of GraphicsContext3D(), we are sure that "this" is
> still a valid pointer but after that it will no longer be valid. So this is
> exactly the right time to remove the raw pointer from the activeContexts().
> 2. This function returns a RefPtr<GraphicsContext3D> which may be
> ref-counted multiple times.  When calling recycleContext(), for a previously
> allocated PtrRef<GraphicsContext3D> by this function, might not actually
> dispose its raw pointer. I am not sure if this can happen or not. But if it
> happens, that means there will be extra contexts hanging around but not
> recorded by activeContexts() because we removed their references from the
> activeContexts() even though their raw pointer are still alive.

The thing is that if the context is lost for other reasons than making room for a new context, then the call to destroyGraphicsContext3D() will not result in any action and thus the destructor will not be called right away. This code doesn't make assumptions about the state of the context.

> > Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:139
> > +    contexts.append(context.get());
> 
> If you decide to remove the  contexts.removeFirst(contextToLose), then you
> will need to check:
> if (contexts.size() >= MaxActiveContexts)
>     return nullptr;
> 
> before this append(). You may also want to move the code which tries to find
> space in activeContexts() before creating the the GraphicsContext3D.
> 
> if (contexts.size() >= MaxActiveContexts) {
>     ...
> }
> 
> if (contexts.size() >= MaxActiveContexts)
>    return nullptr;

I'm not sure I understand this comment. The goal of this patch is to always return a context and recycle older context if need be.

> RefPtr<GraphicsContext3D> context = adoptRef(new GraphicsContext3D(attrs,
> hostWindow, renderStyle));
> 
> > Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:349
> > +    activeContexts().removeFirst(this);
> 
> I think if you have removing the raw pointer from the activeContexts() only
> in the destructor, then you can safely asserts that
> activeContexts().contains(this).

I don't think we can have such an assertion, per my point above about contexts being lost for other reasons than there being too many active contexts, which means calling recycleContext() will not result in the destructor being called. In that case the context may have been removed from the active context list and will only be destructed when GC kicks in at a later time.

> 
> > Source/WebCore/platform/graphics/win/GraphicsContext3DWin.cpp:63
> >          return 0;
> 
> 0 ==> nullptr.

OK. Will change that upon landing if the rest of the patch is fine as-is.
Comment 24 Antoine Quint 2016-04-20 11:10:01 PDT
Created attachment 276836 [details]
Patch
Comment 25 Antoine Quint 2016-04-20 12:17:50 PDT
Actually, Said, I think your comments were correct. I was confused because I used WebGLRenderingContextBase::stop() instead of WebGLRenderingContextBase::destroyGraphicsContext3D() for a while, and that had checks for lost contexts, destroyGraphicsContext3D() does not.
Comment 26 Antoine Quint 2016-04-20 12:27:35 PDT
Created attachment 276843 [details]
Patch
Comment 27 Dean Jackson 2016-04-21 07:32:52 PDT
Comment on attachment 276843 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=276843&action=review

Nearly there. Just some small fixes.

> Source/WebCore/platform/graphics/GraphicsContext3D.h:1269
> +    static const int MaxActiveContexts = 16;

Since this is only used in the GraphicsContext3DMac.cpp file, it doesn't need to be a class attribute. Just use a static variable in the .cpp (like it was before this patch).

Alternatively, we could put it here and encourage other ports to do the same. This would also allow us to be more descriptive in the error message (in Base.cpp). We could say "We only allow %d active contexts at a time.", MaxActiveContexts)

> Source/WebCore/platform/graphics/cairo/GraphicsContext3DCairo.cpp:77
> +    return WTFMove(context);

This should be return context;

Or, you could do it all in one line. return adoptRef(*new Graphics....)

> Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:36
> +    return context->m_private ? WTFMove(context) : 0;

Same here, just return context. And 0 should be nullptr.

> Source/WebCore/platform/graphics/win/GraphicsContext3DWin.cpp:66
> +    return WTFMove(context);

return context here too.
Comment 28 Dean Jackson 2016-04-21 07:42:47 PDT
r=me if you make those changes.
Comment 29 Antoine Quint 2016-04-21 07:46:53 PDT
Created attachment 276925 [details]
Patch
Comment 30 Antoine Quint 2016-04-21 07:49:48 PDT
(In reply to comment #28)
> r=me if you make those changes.

Made the suggested changes.
Comment 31 Dean Jackson 2016-04-21 07:50:06 PDT
Comment on attachment 276925 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=276925&action=review

> Source/WebCore/ChangeLog:28
> +        * html/canvas/WebGLRenderingContextBase.h:

You probably should note that you changed PassRefPtr into RefPtr.
Comment 32 Antoine Quint 2016-04-21 07:53:56 PDT
(In reply to comment #31)
> Comment on attachment 276925 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=276925&action=review
> 
> > Source/WebCore/ChangeLog:28
> > +        * html/canvas/WebGLRenderingContextBase.h:
> 
> You probably should note that you changed PassRefPtr into RefPtr.

Will add this when landing.
Comment 33 Antoine Quint 2016-04-21 08:38:23 PDT
Created attachment 276928 [details]
Patch for landing
Comment 34 WebKit Commit Bot 2016-04-21 09:37:58 PDT
Comment on attachment 276928 [details]
Patch for landing

Clearing flags on attachment: 276928

Committed r199819: <http://trac.webkit.org/changeset/199819>
Comment 35 WebKit Commit Bot 2016-04-21 09:38:04 PDT
All reviewed patches have been landed.  Closing bug.