Bug 220974

<rdar://problem/73876947>

Created attachment 422055 [details]
Patch

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.

Created attachment 423163 [details]
rebase

Created attachment 423164 [details]
rebase

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.
}

Created attachment 423167 [details]
rebase

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

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

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

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