Bug 204343 – Introduce a GPU process

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tim Horton]

Introduce a GPU process

[Tim Horton]

Created attachment 383848 [details]
Patch

[Tim Horton]

Not quite ready for review yet (I haven't tried it on iOS), but pre-review welcome.

It doesn't actually *do* anything, though there is a switch to redirect UI-side compositing transactions through the GPU process (the first step towards doing OOP display list painting for layer contents).

The sandbox profile is stolen from the Web Content process to start, but we can definitely do waaaaay better.

[Tim Horton]

<rdar://problem/56921817>

[EWS Watchlist]

Comment on attachment 383848 [details]
Patch

Attachment 383848 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/13268436

New failing tests:
http/tests/misc/repeat-open-cancel.html

[EWS Watchlist]

Created attachment 383941 [details]
Archive of layout-test-results from ews213 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews213  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Tim Horton]

Created attachment 383986 [details]
Patch

[Tim Horton]

Simon (correctly IMO) thinks we should have a Web Content process side class to coordinate all communication with the GPU process, instead of just messaging it willy-nilly.

[Tim Horton]

Created attachment 384248 [details]
Patch

[Tim Horton]

Created attachment 384270 [details]
Patch

[Tim Horton]

Created attachment 384272 [details]
Patch

[Tim Horton]

Ah, a classic misplaced close curly brace amongst many #ifs.

[Tim Horton]

Created attachment 384324 [details]
Patch

[Tim Horton]

Created attachment 384333 [details]
Patch

[Tim Horton]

Created attachment 384600 [details]
Patch

[Alex Christensen]

Comment on attachment 384600 [details]
Patch

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

Incomplete review.

> Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:86
> +void GPUConnectionToWebProcess::didClose(IPC::Connection& connection)

Unused parameter, extra space.

> Source/WebKit/GPUProcess/GPUConnectionToWebProcess.messages.in:25
> +messages -> GPUConnectionToWebProcess LegacyReceiver {

Please no new legacy receivers.

> Source/WebKit/GPUProcess/GPUProcess.cpp:52
> +static void callExitSoon(IPC::Connection*)

This should not be copied.  It is currently used for the network process, but not the web process.  If we need it for the gpu process, let's call the existing code.

> Source/WebKit/GPUProcess/GPUProcess.cpp:85
> +bool GPUProcess::shouldTerminate()
> +{
> +    return false;
> +}

FIXME here, or comment explaining why?  This seems optimistic.

> Source/WebKit/GPUProcess/GPUProcess.cpp:104
> +        return;
> +}

This looks silly.

> Source/WebKit/GPUProcess/GPUProcess.messages.in:25
> +messages -> GPUProcess LegacyReceiver {

Please no LegacyReceiver.

> Source/WebKit/GPUProcess/EntryPoint/Cocoa/XPCService/GPUServiceEntryPoint.mm:55
> +using namespace WebKit;

Could we not?

> Source/WebKit/GPUProcess/cocoa/GPUProcessCocoa.mm:37
> +void GPUProcess::platformInitializeGPUProcess(const GPUProcessCreationParameters&)
> +{
> +}

Let's just add this as needed.

> Source/WebKit/GPUProcess/ios/GPUProcessIOS.mm:49
> +void GPUProcess::initializeSandbox(const AuxiliaryProcessInitializationParameters&, SandboxInitializationParameters&)

This definitely needs a fixme or not implemented or comment explaining that catalyst will need something here.

> Source/WebKit/UIProcess/GPU/GPUProcessProxy.cpp:198
> +        if (!ProcessThrottler::isValidForegroundActivity(m_activityFromWebProcesses)) {

This should be shared if needed.  I'm not sure what we'll do with background activities and GPU.

> Source/WebKit/UIProcess/GPU/GPUProcessProxy.messages.in:25
> +messages -> GPUProcessProxy LegacyReceiver NotRefCounted {

:(

> Source/WebKit/WebProcess/WebProcess.cpp:1266
> +    if (!connection.sendSync(Messages::WebProcessProxy::GetGPUProcessConnection(), Messages::WebProcessProxy::GetGPUProcessConnection::Reply(connectionInfo), 0)) {

It would be nice if we could avoid a new synchronous message when initializing things.  This could be done in parallel with the network process connection initialization.

> Source/WebKit/WebProcess/GPU/GPUProcessConnection.cpp:65
> +    Ref<GPUProcessConnection> protectedThis(*this);

??

> Source/WebKit/WebProcess/GPU/GPUProcessConnection.messages.in:25
> +messages -> GPUProcessConnection LegacyReceiver {

ditto.  No legacy.
Also, this doesn't seem used in this patch.  Will it be?

[Tim Horton]

> > Source/WebKit/GPUProcess/ios/GPUProcessIOS.mm:49
> > +void GPUProcess::initializeSandbox(const AuxiliaryProcessInitializationParameters&, SandboxInitializationParameters&)
> 
> This definitely needs a fixme or not implemented or comment explaining that
> catalyst will need something here.

It doesn't, it uses the Mac code!! (I hope). This is the same as the network process.

> > Source/WebKit/UIProcess/GPU/GPUProcessProxy.messages.in:25
> > +messages -> GPUProcessProxy LegacyReceiver NotRefCounted {
> 
> :(

Hush, I'll fix them all :)

> > Source/WebKit/WebProcess/WebProcess.cpp:1266
> > +    if (!connection.sendSync(Messages::WebProcessProxy::GetGPUProcessConnection(), Messages::WebProcessProxy::GetGPUProcessConnection::Reply(connectionInfo), 0)) {
> 
> It would be nice if we could avoid a new synchronous message when
> initializing things.  This could be done in parallel with the network
> process connection initialization.

The ChangeLog covers this. There is apparently a new mechanism Coming Soon. I need to check on the status of that.

> > Source/WebKit/WebProcess/GPU/GPUProcessConnection.messages.in:25
> > +messages -> GPUProcessConnection LegacyReceiver {
> 
> Also, this doesn't seem used in this patch.  Will it be?

Definitely! Shortly.

[Tim Horton]

Created attachment 384690 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 384690 [details]
Patch

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

> Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:73
> +void GPUConnectionToWebProcess::didReceiveInvalidMessage(IPC::Connection&, IPC::StringReference, IPC::StringReference)
> +{
> +}

Log or abort or something?

> Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:101
> +void GPUConnectionToWebProcess::willCommitLayerTree(TransactionID transactionID, WebPageProxyIdentifier pageID)
> +{
> +    gpuProcess().parentProcessConnection()->send(Messages::GPUProcessProxy::WillCommitLayerTree(transactionID, pageID), 0);
> +}
> +
> +void GPUConnectionToWebProcess::commitLayerTree(const RemoteLayerTreeTransaction& transaction, const RemoteScrollingCoordinatorTransaction& scrollingTransaction, WebPageProxyIdentifier pageID)
> +{
> +    // FIXME: This work (and most work in this process) should happen
> +    // on a secondary queue.
> +
> +    // We shouldn't need to map all of the IOSurfaces just to send them along
> +    // to the UI process, but just copying the send right doesn't work.
> +    // FIXME: Remove this once we don't actually need IOSurfaces to transit
> +    // this process, once they're just created here.
> +    for (const auto& changedLayerIter : transaction.changedLayerProperties()) {
> +        RemoteLayerTreeTransaction::LayerProperties* properties = changedLayerIter.value.get();
> +        if (!properties->backingStore)
> +            continue;
> +        properties->backingStore->mapBackingStoreIfNeeded();
> +    }
> +
> +    // FIXME: Only forward for page IDs that make sense for this connection.
> +    // This should probably be a MESSAGE_CHECK and kill the misbehaving
> +    // Web Content process.
> +    gpuProcess().parentProcessConnection()->send(Messages::GPUProcessProxy::CommitLayerTree(transaction, scrollingTransaction, pageID), 0);
> +}

Is this the right place for these?

> Source/WebKit/GPUProcess/GPUConnectionToWebProcess.h:36
> +#include <WebCore/PageIdentifier.h>

Unused

> Source/WebKit/GPUProcess/GPUProcess.cpp:117
> +#if USE(UNIX_DOMAIN_SOCKETS)
> +    IPC::Connection::SocketPair socketPair = IPC::Connection::createPlatformConnection();
> +    return std::make_pair(socketPair.server, IPC::Attachment { socketPair.client });
> +#elif OS(DARWIN)
> +    // Create the listening port.
> +    mach_port_t listeningPort = MACH_PORT_NULL;
> +    auto kr = mach_port_allocate(mach_task_self(), MACH_PORT_RIGHT_RECEIVE, &listeningPort);
> +    if (kr != KERN_SUCCESS) {
> +        RELEASE_LOG_ERROR(Process, "GPUProcess::createGPUConnectionToWebProcess: Could not allocate mach port, error %x", kr);
> +        CRASH();
> +    }
> +    if (!MACH_PORT_VALID(listeningPort)) {
> +        RELEASE_LOG_ERROR(Process, "GPUProcess::createGPUConnectionToWebProcess: Could not allocate mach port, returned port was invalid");
> +        CRASH();
> +    }
> +    return std::make_pair(IPC::Connection::Identifier { listeningPort }, IPC::Attachment { listeningPort, MACH_MSG_TYPE_MAKE_SEND });
> +#elif OS(WINDOWS)
> +    IPC::Connection::Identifier serverIdentifier, clientIdentifier;
> +    if (!IPC::Connection::createServerAndClientIdentifiers(serverIdentifier, clientIdentifier)) {
> +        LOG_ERROR("Failed to create server and client identifiers");
> +        CRASH();
> +    }
> +    return std::make_pair(serverIdentifier, IPC::Attachment { clientIdentifier });
> +#else
> +    notImplemented();
> +    return { };
> +#endif

Seems like the kind of thing that should live in a base class.

> Source/WebKit/GPUProcess/GPUProcess.cpp:138
> +    RELEASE_LOG(ProcessSuspension, "%p - GPUProcess::prepareToSuspend(), isSuspensionImminent: %d", this, isSuspensionImminent);

We should probably log enough so we know which GPU Process PID a given pageID is using.

> Source/WebKit/Shared/RemoteLayerTree/RemoteLayerBackingStore.mm:406
> +void RemoteLayerBackingStore::mapBackingStoreIfNeeded()
> +{
> +    if (m_frontBufferSendRight) {
> +        ASSERT(!m_frontBuffer.surface);
> +        m_frontBuffer.surface = WebCore::IOSurface::createFromSendRight(WTFMove(m_frontBufferSendRight), WebCore::sRGBColorSpaceRef());
> +    }
> +}

Maybe not in this patch?

> Source/WebKit/Shared/RemoteLayerTree/RemoteLayerTreeTransaction.mm:544
> -    encoder << static_cast<uint64_t>(m_changedLayers.size());
> +    encoder << static_cast<uint64_t>(m_changedLayers.size() + m_changedLayerProperties.size());
>  
>      for (const auto& layer : m_changedLayers) {
>          encoder << layer->layerID();
>          encoder << layer->properties();
>      }
> +
> +    for (const auto& layerIter : m_changedLayerProperties) {
> +        encoder << layerIter.key;
> +        encoder << *layerIter.value;
> +    }

Not in this patch?

> Source/WebKit/UIProcess/DrawingAreaProxy.h:111
> +    virtual void commitLayerTree(const RemoteLayerTreeTransaction&, const RemoteScrollingCoordinatorTransaction&) { ASSERT_NOT_REACHED(); }

Seems like a layering violation to have *remote* things here.

> Source/WebKit/UIProcess/GPU/GPUProcessProxy.cpp:110
> +void GPUProcessProxy::openGPUProcessConnection(uint64_t connectionRequestIdentifier, WebProcessProxy& webProcessProxy)

Can we make a typedef for connectionRequestIdentifier?

> Source/WebKit/UIProcess/GPU/GPUProcessProxy.cpp:132
> +#if USE(UNIX_DOMAIN_SOCKETS) || OS(WINDOWS)
> +        request.reply(GPUProcessConnectionInfo { WTFMove(*connectionIdentifier) });
> +#elif OS(DARWIN)
> +        MESSAGE_CHECK(MACH_PORT_VALID(connectionIdentifier->port()));
> +        request.reply(GPUProcessConnectionInfo { IPC::Attachment { connectionIdentifier->port(), MACH_MSG_TYPE_MOVE_SEND } });
> +#else
> +        notImplemented();
> +#endif

Should be hidden in a base class.

> Source/WebKit/UIProcess/GPU/GPUProcessProxy.cpp:181
> +    for (const auto& processPool : WebProcessPool::allProcessPools()) {
> +        hasAnyForegroundWebProcesses |= processPool->hasForegroundWebProcesses();
> +        hasAnyBackgroundWebProcesses |= processPool->hasBackgroundWebProcesses();
> +    }

Does this get the right set of clients for this process?

> Source/WebKit/UIProcess/GPU/GPUProcessProxy.cpp:212
> +void GPUProcessProxy::willCommitLayerTree(TransactionID transactionID, WebPageProxyIdentifier pageID)
> +{
> +    auto webPage = WebProcessProxy::webPage(pageID);
> +    webPage->drawingArea()->willCommitLayerTree(transactionID);
> +}
> +
> +void GPUProcessProxy::commitLayerTree(const RemoteLayerTreeTransaction& transaction, const RemoteScrollingCoordinatorTransaction& scrollingTransaction, WebPageProxyIdentifier pageID)
> +{
> +    auto webPage = WebProcessProxy::webPage(pageID);
> +    webPage->drawingArea()->commitLayerTree(transaction, scrollingTransaction);
> +
> +
> +}

Maybe not here (eventually?)

> Source/WebKit/WebProcess/GPU/GPUProcessConnectionInfo.h:46
> +#if USE(UNIX_DOMAIN_SOCKETS)
> +        return IPC::Connection::Identifier(connection.fileDescriptor());
> +#elif OS(DARWIN)
> +        return IPC::Connection::Identifier(connection.port());
> +#elif OS(WINDOWS)
> +        return IPC::Connection::Identifier(connection.handle());
> +#else
> +        ASSERT_NOT_REACHED();
> +        return IPC::Connection::Identifier();
> +#endif

Boo

> Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteLayerTreeDrawingArea.mm:447
> +    Ref<IPC::Connection> connectionForCommits = *WebProcess::singleton().parentProcessConnection();
> +    std::unique_ptr<IPC::Encoder> commitEncoder;
> +
> +    if (m_useGPUProcess) {
> +#if ENABLE(GPU_PROCESS)
> +        connectionForCommits = WebProcess::singleton().ensureGPUProcessConnection().connection();
> +
> +        connectionForCommits->send(Messages::GPUConnectionToWebProcess::WillCommitLayerTree(layerTransaction.transactionID(), m_webPage.webPageProxyIdentifier()), 0);

Maybe all this could be a separate patch

[Tim Horton]

(In reply to Simon Fraser (smfr) from comment #18)
> Comment on attachment 384690 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=384690&action=review
> 
> > Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:73
> > +void GPUConnectionToWebProcess::didReceiveInvalidMessage(IPC::Connection&, IPC::StringReference, IPC::StringReference)
> > +{
> > +}
> 
> Log or abort or something?

Oddly the other children do not, but yes, I'm going to do what the UI process does instead.

> > Source/WebKit/GPUProcess/GPUProcess.cpp:117
> > +#if USE(UNIX_DOMAIN_SOCKETS)
> > +    IPC::Connection::SocketPair socketPair = IPC::Connection::createPlatformConnection();
> > +    return std::make_pair(socketPair.server, IPC::Attachment { socketPair.client 
> Seems like the kind of thing that should live in a base class.

I moved it up and renamed it. It just makes a pair, not specific to the web process.

> > Source/WebKit/GPUProcess/GPUProcess.cpp:138
> > +    RELEASE_LOG(ProcessSuspension, "%p - GPUProcess::prepareToSuspend(), isSuspensionImminent: %d", this, isSuspensionImminent);
> 
> We should probably log enough so we know which GPU Process PID a given
> pageID is using.
> 
> > Source/WebKit/Shared/RemoteLayerTree/RemoteLayerBackingStore.mm:406
> > +void RemoteLayerBackingStore::mapBackingStoreIfNeeded()
> > +{
> > +    if (m_frontBufferSendRight) {
> > +        ASSERT(!m_frontBuffer.surface);
> > +        m_frontBuffer.surface = WebCore::IOSurface::createFromSendRight(WTFMove(m_frontBufferSendRight), WebCore::sRGBColorSpaceRef());
> > +    }
> > +}
> 
> Maybe not in this patch?

I removed all of this stuff.

> > Source/WebKit/UIProcess/DrawingAreaProxy.h:111
> > +    virtual void commitLayerTree(const RemoteLayerTreeTransaction&, const RemoteScrollingCoordinatorTransaction&) { ASSERT_NOT_REACHED(); }
> 
> Seems like a layering violation to have *remote* things here.

Err, yeah, what is wrong with me.

> > Source/WebKit/UIProcess/GPU/GPUProcessProxy.cpp:132
> > +#if USE(UNIX_DOMAIN_SOCKETS) || OS(WINDOWS)
> > +        request.reply(GPUProcessConnectionInfo { WTFMove(*connectionIdentifier) });
> > +#elif OS(DARWIN)
> > +        MESSAGE_CHECK(MACH_PORT_VALID(connectionIdentifier->port()));
> > +        request.reply(GPUProcessConnectionInfo { IPC::Attachment { connectionIdentifier->port(), MACH_MSG_TYPE_MOVE_SEND } });
> > +#else
> > +        notImplemented();
> > +#endif
> 
> Should be hidden in a base class.

Maybe, though the types are unique to each child. Teeeeemplates?

> > Source/WebKit/UIProcess/GPU/GPUProcessProxy.cpp:181
> > +    for (const auto& processPool : WebProcessPool::allProcessPools()) {
> > +        hasAnyForegroundWebProcesses |= processPool->hasForegroundWebProcesses();
> > +        hasAnyBackgroundWebProcesses |= processPool->hasBackgroundWebProcesses();
> > +    }
> 
> Does this get the right set of clients for this process?

This gets the set of ALL web processes for this UI process, which given the POR design are also the entire set of clients for the singleton GPU process. If that changes, we'll have to change this.

> > Source/WebKit/WebProcess/GPU/GPUProcessConnectionInfo.h:46
> > +#if USE(UNIX_DOMAIN_SOCKETS)
> > +        return IPC::Connection::Identifier(connection.fileDescriptor());
> > +#elif OS(DARWIN)
> > +        return IPC::Connection::Identifier(connection.port());
> > +#elif OS(WINDOWS)
> > +        return IPC::Connection::Identifier(connection.handle());
> > +#else
> > +        ASSERT_NOT_REACHED();
> > +        return IPC::Connection::Identifier();
> > +#endif
> 
> Boo

Are you a ghost.

[Tim Horton]

> > > Source/WebKit/UIProcess/GPU/GPUProcessProxy.cpp:132
> > > +#if USE(UNIX_DOMAIN_SOCKETS) || OS(WINDOWS)
> > > +        request.reply(GPUProcessConnectionInfo { WTFMove(*connectionIdentifier) });
> > > +#elif OS(DARWIN)
> > > +        MESSAGE_CHECK(MACH_PORT_VALID(connectionIdentifier->port()));
> > > +        request.reply(GPUProcessConnectionInfo { IPC::Attachment { connectionIdentifier->port(), MACH_MSG_TYPE_MOVE_SEND } });
> > > +#else
> > > +        notImplemented();
> > > +#endif
> > 
> > Should be hidden in a base class.
> 
> Maybe, though the types are unique to each child. Teeeeemplates?

I'm going to leave this alone for this patch and revisit in a follow-up where we perhaps change how the Web Content process gets all of its connections (instead of factoring this out just to delete it).

[Tim Horton]

Created attachment 384790 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 384790 [details]
Patch

Clearing flags on attachment: 384790

Committed r253098: <https://trac.webkit.org/changeset/253098>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.