Bug 220974 – WebGL IPC should use shared memory for synchronous messages

• Component: WebGL
• Status: RESOLVED FIXED

[Kimmo Kinnunen]

WebGL IPC should send synchronised messages through the shared memory.

Also the replies should by default be written to the shared memory.

[Radar WebKit Bug Importer]

<rdar://problem/73876947>

[Kimmo Kinnunen]

Created attachment 422055 [details]
Patch

[Geoffrey Garen]

Comment on attachment 422055 [details]
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.

[Kimmo Kinnunen]

Created attachment 423163 [details]
rebase

[Kimmo Kinnunen]

Created attachment 423164 [details]
rebase

[Kimmo Kinnunen]

Thanks for the review!
(In reply to Geoffrey Garen from comment #3)
> 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?

All the sites expect spurious wakeups from the semaphore waits.
 
> Perhaps it would be better to just do two loads (removing the cocoa-specific
> path), and only store clientIsWaitingTag right before we wait.

That's what the code tries to avoid with the exchange. It is the race the wait structure always has.

E.g. it would be as follows:

for(;;) {
    clientOffset = sharedClientOffset.load();
    clientLimit = sharedClientLimit.load();
    if (!clientOffset && !clientLimit)
        break;

    // Race here: what if the server sets clientOffset = clientLimit = 0 here and goes on to wait
    // for next command from the client?

    sharedClientLimit.store(clientIsWaitingTag);
    wait();  // If the above happens, this waits forever.
}

[Kimmo Kinnunen]

Created attachment 423167 [details]
rebase

[EWS]

Committed r274433: <https://commits.webkit.org/r274433>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 423167 [details].

[Geoffrey Garen]

> All the sites expect spurious wakeups from the semaphore waits.

Got it. I hadn't noticed this before. Thanks!