[webkit-reviews] review granted: [Bug 220974] WebGL IPC should use shared memory for synchronous messages : [Attachment 422055] 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 220974: WebGL IPC should use shared memory for synchronous messages
https://bugs.webkit.org/show_bug.cgi?id=220974

Attachment 422055: Patch

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




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

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

Nice speedup!

I think I may have spotted a small bug.

Can you re-check EWS (and the small bug) before landing?

> Source/WebKit/Platform/IPC/Connection.h:326
> +    bool pushPendingSyncRequestID(uint64_t syncRequestID = 0);

Can we remove the default 0 argument here? Feels like it might invite mistakes.
Seems unused so far.

> Source/WebKit/Platform/IPC/StreamClientConnection.h:284
> +#if PLATFORM(COCOA)
> +	   ClientLimit clientLimit =
sharedClientLimit().exchange(ClientLimit::clientIsWaitingTag,
std::memory_order_acq_rel);
> +	   ClientOffset clientOffset =
sharedClientOffset().load(std::memory_order_acquire);
> +#else
> +	   ClientLimit clientLimit =
sharedClientLimit().load(std::memory_order_acquire);
> +	   ClientOffset clientOffset =
sharedClientOffset().load(std::memory_order_acquire);
> +#endif
> +	   if (!clientLimit && (clientOffset ==
ClientOffset::serverIsSleepingTag || !clientOffset))
> +	       break;

In the iteration where we break out of the loop, we temporarily set
clientIsWaitingTag, but never wait on our semaphore. What happens if the server
notices clientIsWaitingTag, and signals our semaphore, before we have a chance
to clear clientIsWaitingTag? Does that put our semaphore out of balance?

Perhaps it would be better to just do two loads (removing the cocoa-specific
path), and only store clientIsWaitingTag right before we wait.