Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add WorkerClient for WebCore to access WebKit from worker threads, and use it to create a RemoteRenderingBackendProxy per-thread #4085

Conversation

mattwoodrow
Copy link
Contributor

@mattwoodrow mattwoodrow commented Sep 7, 2022

280e6cd

Add WorkerClient for WebCore to access WebKit from worker threads, and use it to create a RemoteRenderingBackendProxy per-thread
https://bugs.webkit.org/show_bug.cgi?id=244828

Reviewed by Kimmo Kinnunen.

Creates a new interface class WorkerClient (which implements GraphicsClient), for accessing WebKit graphics APIs from a worker thread. Adds an implementation
of this in WebWorkerClient.

Allocates an instance of (Web)WorkerClient for every dedicated and shared worker, and makes it available via WorkerGlobalScope.

WebWorkerClient creates a dedicated GPUProcessConnection and RemoteRenderingBackendProx for each worker thread/instance, and uses this to create
RemoteImageBufferProxys that are safe to use on that worker thread.

* Source/WebCore/Headers.cmake:
* Source/WebCore/page/Chrome.cpp:
(WebCore::Chrome::createWorkerClient):
* Source/WebCore/page/Chrome.h:
* Source/WebCore/page/ChromeClient.h:
(WebCore::ChromeClient::createWorkerClient):
* Source/WebCore/page/WorkerClient.h: Added.
* Source/WebCore/workers/DedicatedWorkerGlobalScope.cpp:
(WebCore::DedicatedWorkerGlobalScope::create):
(WebCore::DedicatedWorkerGlobalScope::DedicatedWorkerGlobalScope):
* Source/WebCore/workers/DedicatedWorkerGlobalScope.h:
* Source/WebCore/workers/DedicatedWorkerThread.cpp:
(WebCore::DedicatedWorkerThread::createWorkerGlobalScope):
* Source/WebCore/workers/WorkerGlobalScope.cpp:
(WebCore::WorkerGlobalScope::WorkerGlobalScope):
* Source/WebCore/workers/WorkerGlobalScope.h:
(WebCore::WorkerGlobalScope::workerClient):
* Source/WebCore/workers/WorkerMessagingProxy.cpp:
(WebCore::WorkerMessagingProxy::startWorkerGlobalScope):
* Source/WebCore/workers/WorkerThread.h:
(WebCore::WorkerThread::setWorkerClient):
(WebCore::WorkerThread::workerClient):
* Source/WebCore/workers/service/ServiceWorkerGlobalScope.cpp:
(WebCore::ServiceWorkerGlobalScope::ServiceWorkerGlobalScope):
* Source/WebCore/workers/shared/SharedWorkerGlobalScope.cpp:
(WebCore::SharedWorkerGlobalScope::SharedWorkerGlobalScope):
* Source/WebCore/workers/shared/SharedWorkerGlobalScope.h:
* Source/WebCore/workers/shared/context/SharedWorkerThread.cpp:
(WebCore::SharedWorkerThread::createWorkerGlobalScope):
* Source/WebCore/workers/shared/context/SharedWorkerThreadProxy.cpp:
(WebCore::SharedWorkerThreadProxy::SharedWorkerThreadProxy):
* Source/WebKit/Sources.txt:
* Source/WebKit/WebKit.xcodeproj/project.pbxproj:
* Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp:
(WebKit::WebChromeClient::createWorkerClient):
* Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.h:
* Source/WebKit/WebProcess/WebCoreSupport/WebWorkerClient.cpp: Added.
(WebKit::WebWorkerClient::WebWorkerClient):
(WebKit::WebWorkerClient::ensureRenderingBackend const):
(WebKit::WebWorkerClient::clone):
(WebKit::WebWorkerClient::displayID const):
(WebKit::WebWorkerClient::createImageBuffer const):
* Source/WebKit/WebProcess/WebCoreSupport/WebWorkerClient.h: Added.

Canonical link: https://commits.webkit.org/257507@main

7088983

Misc iOS, tvOS & watchOS macOS Linux Windows
βœ… πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe βœ… πŸ›  πŸ§ͺ win
βœ… πŸ§ͺ bindings βœ… πŸ›  ios-sim βœ… πŸ›  mac-AS-debug βœ… πŸ›  gtk βœ… πŸ›  wincairo
βœ… πŸ§ͺ webkitperl ❌ πŸ§ͺ ios-wk2 βœ… πŸ§ͺ api-mac βœ… πŸ§ͺ gtk-wk2
βœ… πŸ§ͺ api-ios ❌ πŸ§ͺ mac-wk1 βœ… πŸ§ͺ api-gtk
βœ… πŸ›  tv ❌ πŸ§ͺ mac-wk2
βœ… πŸ›  tv-sim ❌ πŸ§ͺ mac-AS-debug-wk2
βœ… πŸ›  πŸ§ͺ merge βœ… πŸ›  watch βœ… πŸ§ͺ mac-wk2-stress
βœ… πŸ›  watch-sim

@mattwoodrow mattwoodrow self-assigned this Sep 7, 2022
@mattwoodrow mattwoodrow added WebKit Misc. For miscellaneous bugs in the WebKit framework (and not JavaScriptCore or WebCore). WebKit Nightly Build labels Sep 7, 2022
@mattwoodrow
Copy link
Contributor Author

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Sep 7, 2022
@kkinnunen-apple
Copy link
Contributor

I think this work could be simplified to first pursue the changes needed with GraphicsClient.

It seems that we are lacking a per-worker factory for polymorphic object instantiations.
We have per-page factory ChromeClient that creates objects that depend on global state.
We need per-worker factory of similar sort, WorkerClient.

I would start with:

  1. Extract what ImageBuffer/WebGL needs from ChromeClient to GraphicsClient, make it work with current code.

2a. Investigate what is the object that workers use to configure the polymorphic objects via the global state. If there is none, add this interface

alternatively

2b. Add logic for ChromeClient to hold per-thread GraphicsClients

Small detail:
I suppose the ScriptExecutionContext would be the place to add the initial polymorphic query to get the GraphicsClient?

E.g. there's ScriptExecutionContext::notificationClient(), and similar to this, there could be one for graphics?

Things that initially do not make sense to me:

GPUProcess updated to handle multiple instances of GPUConnectionToWebProcess per web process

This does not seem to be needed. To me, it appears as if we just need a RemoteRenderingBackendProxy per worker. Then we just need to make sure that the closure/crash support works if it doesn't work with the main connection.

SerialDispatchQueue -- We already have WorkQueue, which is a serial dispatch queue. So the code about that doesn't make sense initially. Note: the implementation seems to be missing from this patch.

@mattwoodrow
Copy link
Contributor Author

Things that initially do not make sense to me:

GPUProcess updated to handle multiple instances of GPUConnectionToWebProcess per web process

This does not seem to be needed. To me, it appears as if we just need a RemoteRenderingBackendProxy per worker. Then we just need to make sure that the closure/crash support works if it doesn't work with the main connection.

We'd also need to add support to IPC::Connection for sending sync messages from the worker thread, and make sure responses are handled without touching/blocking the main thread. That's pretty complicated, with all the complexity we already have for handling unrelated messages while blocking for a sync message.

Instead, I added support for creating new IPC::Connections that are bound to the worker thread (and can send sync messages from that thread), but don't support the re-entrant message handling (for simplicity, and we don't need that functionality for the current use case).

This does require creating a new GPUConnectionToWebProcess though, bound to the IPC::Connection for the worker.

This is all in #4043 (which is a dependency here, though GitHub doesn't let me express that in a useful way).

SerialDispatchQueue -- We already have WorkQueue, which is a serial dispatch queue. So the code about that doesn't make sense initially. Note: the implementation seems to be missing from this patch.

The implementation is in #4043.

The functionality we're trying to represent here is 'the main thread, or a worker', that we can dispatch tasks to, assert that we're currently running in the expected place, and hold on to a reference to (could be weak, or strong).

WorkQueue is a specific implementation, that neither the main thread or workers are. The closest existing thing that I can find is FunctionDispatcher (which isn't implemented by WorkerOrWorkletThread, but can be), but that doesn't offer refcounting, or assertions.

@mattwoodrow
Copy link
Contributor Author

SerialDispatchQueue -- We already have WorkQueue, which is a serial dispatch queue. So the code about that doesn't make sense initially. Note: the implementation seems to be missing from this patch.

Note that WorkQueue could implement the SerialDispatchQueue interface, but ConcurrentWorkQueue can't really, so it's a bit tricky.

@mattwoodrow mattwoodrow removed the merging-blocked Applied to prevent a change from being merged label Oct 11, 2022
@mattwoodrow mattwoodrow force-pushed the eng/rendering-backend-per-thread branch from f71351e to 73a4c71 Compare October 11, 2022 04:17
@mattwoodrow
Copy link
Contributor Author

Please note, only the top two commits are truly part of this PR. The others are just there because GitHub (and EWS) don't support dependent branches.

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Oct 11, 2022
@mattwoodrow mattwoodrow removed the merging-blocked Applied to prevent a change from being merged label Oct 19, 2022
@mattwoodrow mattwoodrow force-pushed the eng/rendering-backend-per-thread branch from 73a4c71 to 03e85a2 Compare October 19, 2022 04:25
@mattwoodrow mattwoodrow force-pushed the eng/rendering-backend-per-thread branch from 03e85a2 to 2519ee1 Compare October 19, 2022 21:54
@mattwoodrow mattwoodrow force-pushed the eng/rendering-backend-per-thread branch from 2519ee1 to cb76e02 Compare October 20, 2022 00:41
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Oct 20, 2022
@mattwoodrow mattwoodrow removed the merging-blocked Applied to prevent a change from being merged label Oct 20, 2022
@mattwoodrow mattwoodrow force-pushed the eng/rendering-backend-per-thread branch from cb76e02 to bd97b32 Compare October 20, 2022 02:15
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Oct 20, 2022
@mattwoodrow mattwoodrow removed the merging-blocked Applied to prevent a change from being merged label Oct 27, 2022
@mattwoodrow mattwoodrow force-pushed the eng/rendering-backend-per-thread branch from 510d83b to 7844c4a Compare October 27, 2022 03:18
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Oct 27, 2022
@mattwoodrow mattwoodrow removed the merging-blocked Applied to prevent a change from being merged label Oct 27, 2022
@mattwoodrow mattwoodrow force-pushed the eng/rendering-backend-per-thread branch from 7844c4a to 4904912 Compare October 27, 2022 19:03
Source/WebCore/workers/WorkerGlobalScope.h Outdated Show resolved Hide resolved
Source/WebCore/html/ImageBitmap.cpp Outdated Show resolved Hide resolved
Source/WebCore/html/OffscreenCanvas.cpp Outdated Show resolved Hide resolved
Source/WebCore/html/OffscreenCanvas.cpp Outdated Show resolved Hide resolved
Source/WebCore/platform/GraphicsClient.h Outdated Show resolved Hide resolved
Source/WebKit/WebProcess/WebProcess.cpp Outdated Show resolved Hide resolved
class WebPage;
class RemoteRenderingBackendProxy;

class WebWorkerClient : public WebCore::WorkerClient {
Copy link
Contributor

@shallawa shallawa Oct 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are many #if ENABLE(GPU_PROCESS) in this header file and in the source file. Should not the GPUP case be moved to a new class named RemoteWebWorkerClient which inherits from WebWorkerClient ? WebChromeClient::createWorkerClient() will make the decision from the beginning so we do not have to deal with two different code paths in the same file.


RefPtr<ImageBuffer> ImageBuffer::sinkIntoBufferForDifferentThread()
{
ASSERT(hasOneRef());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this always true? And what is the point in asserting it hasOneRef() and then create a new RefPtr of the ImageBuffer which will increment the ref count?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above, the naming might not be great here.

It asserts that there's only one ref, because it's a private virtual and the only caller is the static which also asserts that it owns the only ref.

The default behaviour is that if the static owns the only ref to the ImageBuffer, then we can allow it to be passed to a new thread as-is since there won't be any other cross-thread references.

Source/WebKit/GPUProcess/GPUProcess.cpp Outdated Show resolved Hide resolved
Source/WebKit/GPUProcess/GPUProcess.cpp Outdated Show resolved Hide resolved
@mattwoodrow mattwoodrow force-pushed the eng/rendering-backend-per-thread branch from 4904912 to 9d2f63b Compare October 27, 2022 23:17
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Oct 30, 2022
@mattwoodrow mattwoodrow force-pushed the eng/rendering-backend-per-thread branch from 9d2f63b to 3aba2e8 Compare November 9, 2022 00:41
@mattwoodrow mattwoodrow removed the merging-blocked Applied to prevent a change from being merged label Nov 28, 2022
@mattwoodrow mattwoodrow force-pushed the eng/rendering-backend-per-thread branch from 3aba2e8 to 5d21d44 Compare November 28, 2022 21:21
@mattwoodrow mattwoodrow force-pushed the eng/rendering-backend-per-thread branch from 5d21d44 to 7088983 Compare December 2, 2022 05:34
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Dec 2, 2022
Copy link
Contributor

@kkinnunen-apple kkinnunen-apple left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To solve the most of the ifdefing problems, you can obtain the data needed by RemoteRenderingBackendCreationParameters expanded to the constructor args.

@mattwoodrow mattwoodrow added merge-queue Applied to send a pull request to merge-queue and removed merging-blocked Applied to prevent a change from being merged labels Dec 7, 2022
…d use it to create a RemoteRenderingBackendProxy per-thread

https://bugs.webkit.org/show_bug.cgi?id=244828

Reviewed by Kimmo Kinnunen.

Creates a new interface class WorkerClient (which implements GraphicsClient), for accessing WebKit graphics APIs from a worker thread. Adds an implementation
of this in WebWorkerClient.

Allocates an instance of (Web)WorkerClient for every dedicated and shared worker, and makes it available via WorkerGlobalScope.

WebWorkerClient creates a dedicated GPUProcessConnection and RemoteRenderingBackendProx for each worker thread/instance, and uses this to create
RemoteImageBufferProxys that are safe to use on that worker thread.

* Source/WebCore/Headers.cmake:
* Source/WebCore/page/Chrome.cpp:
(WebCore::Chrome::createWorkerClient):
* Source/WebCore/page/Chrome.h:
* Source/WebCore/page/ChromeClient.h:
(WebCore::ChromeClient::createWorkerClient):
* Source/WebCore/page/WorkerClient.h: Added.
* Source/WebCore/workers/DedicatedWorkerGlobalScope.cpp:
(WebCore::DedicatedWorkerGlobalScope::create):
(WebCore::DedicatedWorkerGlobalScope::DedicatedWorkerGlobalScope):
* Source/WebCore/workers/DedicatedWorkerGlobalScope.h:
* Source/WebCore/workers/DedicatedWorkerThread.cpp:
(WebCore::DedicatedWorkerThread::createWorkerGlobalScope):
* Source/WebCore/workers/WorkerGlobalScope.cpp:
(WebCore::WorkerGlobalScope::WorkerGlobalScope):
* Source/WebCore/workers/WorkerGlobalScope.h:
(WebCore::WorkerGlobalScope::workerClient):
* Source/WebCore/workers/WorkerMessagingProxy.cpp:
(WebCore::WorkerMessagingProxy::startWorkerGlobalScope):
* Source/WebCore/workers/WorkerThread.h:
(WebCore::WorkerThread::setWorkerClient):
(WebCore::WorkerThread::workerClient):
* Source/WebCore/workers/service/ServiceWorkerGlobalScope.cpp:
(WebCore::ServiceWorkerGlobalScope::ServiceWorkerGlobalScope):
* Source/WebCore/workers/shared/SharedWorkerGlobalScope.cpp:
(WebCore::SharedWorkerGlobalScope::SharedWorkerGlobalScope):
* Source/WebCore/workers/shared/SharedWorkerGlobalScope.h:
* Source/WebCore/workers/shared/context/SharedWorkerThread.cpp:
(WebCore::SharedWorkerThread::createWorkerGlobalScope):
* Source/WebCore/workers/shared/context/SharedWorkerThreadProxy.cpp:
(WebCore::SharedWorkerThreadProxy::SharedWorkerThreadProxy):
* Source/WebKit/Sources.txt:
* Source/WebKit/WebKit.xcodeproj/project.pbxproj:
* Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp:
(WebKit::WebChromeClient::createWorkerClient):
* Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.h:
* Source/WebKit/WebProcess/WebCoreSupport/WebWorkerClient.cpp: Added.
(WebKit::WebWorkerClient::WebWorkerClient):
(WebKit::WebWorkerClient::ensureRenderingBackend const):
(WebKit::WebWorkerClient::clone):
(WebKit::WebWorkerClient::displayID const):
(WebKit::WebWorkerClient::createImageBuffer const):
* Source/WebKit/WebProcess/WebCoreSupport/WebWorkerClient.h: Added.

Canonical link: https://commits.webkit.org/257507@main
@webkit-commit-queue webkit-commit-queue merged commit 280e6cd into WebKit:main Dec 7, 2022
@webkit-commit-queue
Copy link
Collaborator

Committed 257507@main (280e6cd): https://commits.webkit.org/257507@main

Reviewed commits have been landed. Closing PR #4085 and removing active labels.

@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Dec 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WebKit Misc. For miscellaneous bugs in the WebKit framework (and not JavaScriptCore or WebCore).
Projects
None yet
7 participants