Created attachment 423726 [details] Patch

Comment on attachment 423726 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423726&action=review > Source/WebCore/ChangeLog:11 > + This can be done when run in WebContent process or in GPU process, > + but not when in WK1. Why? Could you explain this here? > Source/WebCore/platform/graphics/cocoa/GraphicsContextGLOpenGLCocoa.mm:55 > +static bool isCurrentContextPredictable() > +{ > + static bool value = isInWebProcess() || isInGPUProcess(); > + return value; > +} This is pretty mysterious. What is it about being in the web process or GPU process that makes this possible? Is it that we know there are no other clients of ANGLE?

Why can't ANGLE just do this optimization for us?

WK1 runs third-party code in the thread that it runs WebKit code. That code can switch EAGL contexts behind WebKit's/ANGLE's back. That's what the existing volatile context code already is (and as such there's no *new* emphasis about the issue)

(In reply to Simon Fraser (smfr) from comment #3) > Why can't ANGLE just do this optimization for us? ANGLE needs to be thread safe and there's other semantics to MakeCurrent. (ANGLE doesn't necessarily respect all the other semantics, though). It's a bit surprising the first point is such expensive. EGLBoolean EGLAPIENTRY EGL_MakeCurrent(EGLDisplay dpy, EGLSurface draw, EGLSurface read, EGLContext ctx) { ANGLE_SCOPED_GLOBAL_LOCK(); FUNC_EVENT("EGLDisplay dpy = 0x%016" PRIxPTR ", EGLSurface draw = 0x%016" PRIxPTR ", EGLSurface read = 0x%016" PRIxPTR ", " "EGLContext ctx = %d", (uintptr_t)dpy, (uintptr_t)draw, (uintptr_t)read, CID(dpy, ctx)); Thread *thread = egl::GetCurrentThread(); egl::Display *display = static_cast<egl::Display *>(dpy); Surface *drawSurface = static_cast<Surface *>(draw); Surface *readSurface = static_cast<Surface *>(read); gl::Context *context = static_cast<gl::Context *>(ctx); ANGLE_EGL_TRY_RETURN(thread, ValidateMakeCurrent(display, drawSurface, readSurface, context), "eglMakeCurrent", GetContextIfValid(display, context), EGL_FALSE); ANGLE_EGL_TRY_RETURN(thread, display->prepareForCall(), "eglMakeCurrent", GetDisplayIfValid(display), EGL_FALSE); Surface *previousDraw = thread->getCurrentDrawSurface(); Surface *previousRead = thread->getCurrentReadSurface(); gl::Context *previousContext = thread->getContext(); // Only call makeCurrent if the context or surfaces have changed. if (previousDraw != drawSurface || previousRead != readSurface || previousContext != context) { ANGLE_EGL_TRY_RETURN(thread, display->makeCurrent(thread, drawSurface, readSurface, context), "eglMakeCurrent", GetContextIfValid(display, context), EGL_FALSE); SetContextCurrent(thread, context); } thread->setSuccess(); return EGL_TRUE; }

Comment on attachment 423726 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423726&action=review I agree with Simon that it's unfortunate this optimization is needed. Would you consider reaching out to the ANGLE developers on Slack to see whether we can instead optimize ANGLE's EGL_MakeCurrent? Maybe we can instead optimize its global lock management and/or thread-local variables using cheaper intrinsics to maintain complete correctness and still get a good speed boost. Regardless, setting r+ to unblock you since this is a significant performance increase. Also - agree that moving to using ANGLE's explicit context entry points would be great, and probably a big win. > Source/WebCore/platform/graphics/cocoa/GraphicsContextGLOpenGLCocoa.mm:49 > +static GraphicsContextGLOpenGL* currentContext; As soon as WebKit supports rendering to OffscreenCanvas from web workers, this optimization will break. Are you sure you want to go in this direction? At least I think it would be worth filing a bug and adding a FIXME or similar about it.

Comment on attachment 423726 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423726&action=review > Source/WebCore/platform/graphics/cocoa/GraphicsContextGLOpenGLCocoa.mm:428 > + if (!m_needsMakeCurrent) Actually, hang on. If you're rendering WebGL into two different canvases simultaneously and alternating the calls between the contexts then you have to check if this == currentContext as the first condition, right?

(Removed my r+ because not sure this is correct any more)

(In reply to Kenneth Russell from comment #6) > Also - agree that moving to using ANGLE's explicit context entry points > would be great, and probably a big win. That's what I thought, but looking at the explicit context implementation, isn't it just side-stepping a global variable load ? (I've fixed the bugs in WebKit ANGLE integration to be able to use explicit context) > > > Source/WebCore/platform/graphics/cocoa/GraphicsContextGLOpenGLCocoa.mm:49 > > +static GraphicsContextGLOpenGL* currentContext; > > As soon as WebKit supports rendering to OffscreenCanvas from web workers, > this optimization will break. Are you sure you want to go in this direction? > At least I think it would be worth filing a bug and adding a FIXME or > similar about it. You mean as soon as WebKit supports rendering to OffscreenCanvas from web worker which is executing in different thread? I don't think we have multi-threaded ANGLE, right? E.g. in that scenario, ANGLE cannot be called into from simultaneous threads, making this point moot?

Comment on attachment 423726 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423726&action=review > Source/WebCore/platform/graphics/cocoa/GraphicsContextGLOpenGLCocoa.mm:435 > + currentContext->m_needsMakeCurrent = true; When the target canvas changes, this is intended to take care of the case you're indicating.

Created attachment 423866 [details] Patch

I think the previous was correct but it rised too many questions for it to be ok. How about this one? I replaced the m_needsMakeCurrent with a comment explaining why it's ok to access the global variable.

Comment on attachment 423866 [details] Patch Thanks for the update. I didn't understand how the previous code worked because there's a separate GraphicsContextGL instance for each WebGLRenderingContext, but this is much clearer. r+ Hopefully the webgl/conformance/extensions/khr-parallel-shader-compile.html failure on ios-wk2, unrelated to this, has been suppressed.

(In reply to Kimmo Kinnunen from comment #9) > You mean as soon as WebKit supports rendering to OffscreenCanvas from web > worker which is executing in different thread? > > I don't think we have multi-threaded ANGLE, right? E.g. in that scenario, > ANGLE cannot be called into from simultaneous threads, making this point > moot? I see now (more clearly from the latest patch set) that this is hidden entirely in the Cocoa implementation of GraphicsContextGL, so will go away when using the GPU process. Never mind this concern then. I agree we won't have full multi-threaded ANGLE any time soon nor need to use it for this scenario.

<rdar://problem/75884571>

Committed r275097: <https://commits.webkit.org/r275097> All reviewed patches have been landed. Closing bug and clearing flags on attachment 423866 [details].