[webkit-reviews] review granted: [Bug 204343] Introduce a GPU process : [Attachment 384690] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Tim Horton
<thorton at apple.com>'s request for review:
Bug 204343: Introduce a GPU process
https://bugs.webkit.org/show_bug.cgi?id=204343

Attachment 384690: Patch

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




--- Comment #18 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 384690
  --> https://bugs.webkit.org/attachment.cgi?id=384690
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::WillCom
mitLayerTree(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::CommitL
ayerTree(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::WillCommitLayer
Tree(layerTransaction.transactionID(), m_webPage.webPageProxyIdentifier()), 0);

Maybe all this could be a separate patch