[webkit-reviews] review granted: [Bug 219641] WebGL GPU process IPC should use shared memory for asynchronous messages : [Attachment 420824] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Kimmo Kinnunen
<kkinnunen at apple.com>'s request for review:
Bug 219641: WebGL GPU process IPC should use shared memory for asynchronous
messages
https://bugs.webkit.org/show_bug.cgi?id=219641

Attachment 420824: Patch

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




--- Comment #39 from Geoffrey Garen <ggaren at apple.com> ---
Comment on attachment 420824
  --> https://bugs.webkit.org/attachment.cgi?id=420824
Patch

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

r=me

I wasn't able to review the synchronization details as thoroughly as I'd like,
but I guess we can refine more once this is in-tree.

EWS failures seem like they were caused by an unrelated transient bug in media
streams. Still, I'd suggest re-uploading and hitting cq+ to make sure this
lands cleanly.

> Source/WebKit/Platform/IPC/StreamClientConnection.h:167
> +    // Not notifying on wake up since the out-of-stream message will do
that.
> +    (void) release(encoder.size());

WebKit style is to use the UNUSED_VARIABLE macro here.

> Source/WebKit/Platform/IPC/StreamClientConnection.h:193
> +    return WTF::nullopt;

Are there any valid conditions under which we would receive some data, but not
enough data to be a valid message? If so, let's document them here. If not,
let's ASSERT.

> Source/WebKit/Platform/IPC/StreamClientConnection.h:205
> +    if (receiverLimit != expectedReceiverLimit)
> +	   return WakeUpReceiver::Yes;

I think this would be clearer as "if (receiverLimit &
senderOffsetReceiverIsSleepingTag)". Maybe also add an ASSERT that
(receiverLimit & ~senderOffsetReceiverIsSleepingTag) == expectedReceiverLimit.

> Source/WebKit/Platform/IPC/StreamServerConnection.cpp:89
> +    size_t oldReceiverOffset =
sharedReceiverOffset().exchange(receiverOffset, std::memory_order_acq_rel);
> +    // If the sender wrote over receiverOffset, it means the sender is
waiting.
> +    if (oldReceiverOffset != m_receiverOffset)

I think this would be clearer as "if (oldReceiverOffset &
receiverOffsetSenderIsWaitingTag)". Same comment about adding an assert.