[webkit-reviews] review granted: [Bug 220974] WebGL IPC should use shared memory for synchronous messages : [Attachment 422055] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Mar 10 14:14:01 PST 2021

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

Attachment 422055: Patch


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

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
> +	   ClientLimit clientLimit =
> +	   ClientOffset clientOffset =
> +#else
> +	   ClientLimit clientLimit =
> +	   ClientOffset clientOffset =
> +#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.

More information about the webkit-reviews mailing list