Bug 219641 - WebGL GPU process IPC should use shared memory for asynchronous messages
Summary: WebGL GPU process IPC should use shared memory for asynchronous messages
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kimmo Kinnunen
URL:
Keywords: InRadar
Depends on: 220519 221396 221488 221560
Blocks: webglgpup 218184 220974
  Show dependency treegraph
 
Reported: 2020-12-08 06:48 PST by Kimmo Kinnunen
Modified: 2021-02-22 09:37 PST (History)
29 users (show)

See Also:


Attachments
Patch (171.41 KB, patch)
2020-12-08 07:02 PST, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch (171.08 KB, patch)
2020-12-08 07:07 PST, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch (178.60 KB, patch)
2020-12-11 07:17 PST, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch (182.31 KB, patch)
2020-12-15 06:18 PST, Kimmo Kinnunen
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (182.61 KB, patch)
2020-12-15 06:45 PST, Kimmo Kinnunen
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (182.61 KB, patch)
2020-12-15 07:10 PST, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch (256.15 KB, patch)
2020-12-18 11:21 PST, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch (63.59 KB, patch)
2021-01-26 06:01 PST, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch (342.89 KB, patch)
2021-02-01 13:06 PST, Kimmo Kinnunen
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (347.20 KB, patch)
2021-02-01 13:12 PST, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
rebased to tot, not fully split (164.35 KB, patch)
2021-02-05 10:29 PST, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
removed ipc message receive queue (144.13 KB, patch)
2021-02-09 05:44 PST, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
ready for review (147.46 KB, patch)
2021-02-09 06:33 PST, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch (159.41 KB, patch)
2021-02-17 01:01 PST, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch (160.15 KB, patch)
2021-02-17 02:27 PST, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch (160.12 KB, patch)
2021-02-17 03:34 PST, Kimmo Kinnunen
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (159.61 KB, patch)
2021-02-17 03:47 PST, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch (159.83 KB, patch)
2021-02-17 10:59 PST, Kimmo Kinnunen
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (159.83 KB, patch)
2021-02-17 11:14 PST, Kimmo Kinnunen
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (159.83 KB, patch)
2021-02-17 11:26 PST, Kimmo Kinnunen
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (159.80 KB, patch)
2021-02-17 11:46 PST, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch (160.28 KB, patch)
2021-02-18 00:11 PST, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch (160.84 KB, patch)
2021-02-18 06:00 PST, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch (162.68 KB, patch)
2021-02-19 01:40 PST, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch (164.20 KB, patch)
2021-02-19 04:41 PST, Kimmo Kinnunen
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (164.37 KB, patch)
2021-02-19 06:01 PST, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch (164.48 KB, patch)
2021-02-19 22:22 PST, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch for landing (164.20 KB, patch)
2021-02-20 12:20 PST, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kimmo Kinnunen 2020-12-08 06:48:35 PST
WebGL GPU process IPC should be faster than current WebKit IPC

Most likely design involves putting the remote graphics context method calls into a shared memory until a synchronisation point such as synchronised method call is invoked.
Comment 1 Kimmo Kinnunen 2020-12-08 07:02:44 PST
Created attachment 415640 [details]
Patch
Comment 2 Kimmo Kinnunen 2020-12-08 07:07:24 PST
Created attachment 415641 [details]
Patch
Comment 3 Sam Weinig 2020-12-10 11:51:24 PST
Seems like this is trying to solve a very similar problem to the problem Wenson is solving for DisplayList IPC. Rather than having two divergent implementations it would be much better to unify on a single set of primitives. 

(Even more ideally, if it turns out we can improve all IPC using a different model than we currently use, we should look into that, but it may be that for most uses the ergonomics of the existing approach make more sense).
Comment 4 Kimmo Kinnunen 2020-12-11 07:17:14 PST
Created attachment 415993 [details]
Patch
Comment 5 Geoffrey Garen 2020-12-11 10:22:12 PST
> Rather than having two divergent
> implementations it would be much better to unify on a single set of
> primitives. 

What, specifically, do you think is the single set of primitives that would be much better?

How, specifically, do you think we should evaluate whether that set of primitives is, in fact, much better?
Comment 6 Sam Weinig 2020-12-11 11:35:03 PST
(In reply to Geoffrey Garen from comment #5)
> > Rather than having two divergent
> > implementations it would be much better to unify on a single set of
> > primitives. 
> 
> What, specifically, do you think is the single set of primitives that would
> be much better?

That's a really tough question, and one I am not able to answer. I am raising a concern here that what I see looks like a lot of code that is duplicating logic that is particularly tricky to get to right, and that feels worth bringing up.

Given this seems to be about WebGL, which has relatively similar characteristics to a 2d graphics context, I think a good starting point would be trying to generalize Wenson and others' shared memory display list work.


> How, specifically, do you think we should evaluate whether that set of
> primitives is, in fact, much better?

This is would be subjective, but in my experience, reducing the number of ways to do similar things, and breaking them down into manageable re-usable components makes for an easier time building new things on top of them. And when it seems like we are duplicating things, taking a step back, and seeing if there is a collaboration that can be done can be useful.

There are of course times when multiple similar data structures or approaches are the right decision, whether for specialized memory or performance or compatibility reasons, but those, again in my experience, tend to be the exception and not the rule.
Comment 7 Geoffrey Garen 2020-12-11 12:37:30 PST
> > What, specifically, do you think is the single set of primitives that would
> > be much better?
> 
> That's a really tough question, and one I am not able to answer. I am
> raising a concern here that what I see looks like a lot of code that is
> duplicating logic that is particularly tricky to get to right, and that
> feels worth bringing up.

What, specifically, has been duplicated?

How, specifically, should it be de-duplicated?

> Given this seems to be about WebGL, which has relatively similar
> characteristics to a 2d graphics context, I think a good starting point
> would be trying to generalize Wenson and others' shared memory display list
> work.

'staring point' seems to imply that nothing else comes before it. That would mean setting aside this patch.

A few problems with that proposal:

(1) Setting aside (mostly) working WebGL code has the concrete downside that it moves progress backwards on WebGL.

(2) The shared memory display list is currently a performance regression. This bug report is about getting a speedup. Standardizing on a regression is not a good way to get a speedup.

(3) MessageStreamReceiver, being lower level and more generic, is more likely in my opinion than the shared memory display list to be a reusable abstraction. So, the proposal may defeat its own goal statement.

(4) The proposal is not falsifiable. It does not include any condition under which we could change course and choose not to share code, nor any test for such a condition. In my experience, software and software engineering practices are more successful when they are tested.

(5) Sharing does not include any stated upside. "easier time building new things on top of them" comes close, but doesn't specify what else we need to build on top, or why it matters. The obvious counter-argument is YAGNI.
Comment 8 Geoffrey Garen 2020-12-14 14:09:05 PST
Comment on attachment 415993 [details]
Patch

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

> Source/WebKit/ChangeLog:28
> +        stream andm processing will continue once the out-of-line

and

> Source/WebKit/ChangeLog:36
> +         - Simple things work
> +         - Buggy in certain conditions (asserts/crashes)

Seems like this is not ready to land yet? I'll mark it r- for now to indicate it is not ready.

> Source/WebKit/ChangeLog:48
> +        No new tests, tested by existing tests.

Do we have preliminary perf data from this patch?

> Source/WebKit/Platform/IPC/Decoder.cpp:195
> -    
> +

Whitespace change

> Source/WebKit/Platform/IPC/Decoder.h:58
> +    static constexpr size_t streamMessageAlignment = alignof(uint64_t);

What are we trying to achieve with this alignment setting? Is the goal to enforce a minimum alignment sufficient to read any type?

> Source/WebKit/Platform/IPC/Decoder.h:59
> +    static constexpr size_t outOfLineStreamMessageSize = sizeof(uint64_t) * 2;

Why 2?

> Source/WebKit/Platform/IPC/Decoder.h:161
> -    
> +

Whitespace change

> Source/WebKit/Platform/IPC/DestinationStream.h:39
> +class DestinationStreamBase {

I think this might read more clearly as a data member named DestinationStream (or just Stream), rather than a base clase. A stream reader/writer has a stream to read/write, but is not itself a stream.

> Source/WebKit/Platform/IPC/DestinationStream.h:51
> +        uint64_t cacheLinePadding[2];
> +        Atomic<size_t> writeOffset;

I think you can use alignas instead of cacheLinePadding.

> Source/WebKit/Platform/IPC/DestinationStream.h:57
> +    size_t adjustOffset(size_t offset) {

This function could use a more specific name. I think the adjustment being performed here is wrapping, right?

> Source/WebKit/Platform/IPC/DestinationStream.h:71
> +    static constexpr size_t kReadLimitSleepTag = 1 << 31;

I think this indicates that the reader is sleeping, so I would call it kReaderIsSleepingTag instead.

But also: commitWrite() doesn't seem to consult this tag. Is that a bug?

> Source/WebKit/Platform/IPC/DestinationStream.h:107
> +    void write(uint8_t*& ptr, size_t& capacity)
> +    {
> +        size_t writeLimit = std::min(header().readOffset.load(std::memory_order_acquire), m_dataSize);
> +        alignedSpan(ptr, capacity, m_writeOffset, writeLimit);
> +    }
> +    void writep(uint8_t*& ptr, size_t& capacity)
> +    {
> +        size_t writeLimit = std::min(header().readOffset.load(std::memory_order_acquire), m_dataSize);
> +        alignedSpan(ptr, capacity, m_writeOffset, writeLimit);
> +    }

What's the difference between these two functions?

> Source/WebKit/Platform/IPC/DestinationStream.h:120
> +        if (readLimit != expectedReadLimit)
> +            return WakeUpReader::Yes;
> +        return WakeUpReader::No;

Should we check kReadLimitSleepTag here?

> Source/WebKit/Platform/IPC/Encoder.h:148
> -    
> +

Whitespace change

> Source/WebKit/Platform/IPC/MessageStreamReceiver.h:51
> +            ASSERT(decoder.isValid());

Probably need a stronger check here.

> Source/WebKit/Platform/IPC/MessageStreamSender.h:52
> +            Thread::yield();

This is probably OK for now, but I think we should switch to using a semaphore (for both reading and writing). See discussion in https://bugs.webkit.org/show_bug.cgi?id=219586.

> Source/WebKit/Platform/IPC/MessageStreamSender.h:64
> +            auto wakeupResult = m_stream.commitWrite(Encoder::outOfLineStreamMessageSize);

Since the StreamMessageOutOfLine message is a flag, why does it need size at all? Are we trying to overload the read operation in some way? If so, can we do something more explicit instead?
Comment 9 Kimmo Kinnunen 2020-12-15 06:18:07 PST
Created attachment 416239 [details]
Patch
Comment 10 Kimmo Kinnunen 2020-12-15 06:45:22 PST
Created attachment 416240 [details]
Patch
Comment 11 Radar WebKit Bug Importer 2020-12-15 06:49:21 PST
<rdar://problem/72340651>
Comment 12 Antti Koivisto 2020-12-15 06:54:09 PST
Comment on attachment 416239 [details]
Patch

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

Very cool

> Source/WebKit/Platform/IPC/DestinationStream.h:50
> +// DestinationStreamSender and DestinationStremaReceiver use StreamBuffer to communicate.

typo 'DestinationStremaReceiver'

> Source/WebKit/Platform/IPC/DestinationStream.h:214
> +    size_t len(size_t offset, size_t limit)

WebKit generally prefers full words, length instead len

> Source/WebKit/Platform/IPC/DestinationStream.h:299
> +    size_t len(size_t offset, size_t limit)

here too

> Source/WebKit/Platform/IPC/Encoder.cpp:225
> -
> -    std::memset(m_buffer + m_bufferSize, 0, alignedSize - m_bufferSize);
> -
> +#if 0
> +    if (m_bufferSize < alignedSize)
> +        std::memset(m_buffer + m_bufferSize, 0, alignedSize - m_bufferSize);
> +#endif

some disabled code here

> Source/WebKit/Platform/IPC/MessageStreamSender.h:61
> +            encoder = makeUnique<Encoder>(Messages::RemoteGraphicsContextGL::StreamWakeUp::name(), destinationID);

I guess RemoteGraphicsContextGL here should be something more generic in the future.
Comment 13 Kimmo Kinnunen 2020-12-15 07:02:29 PST
(In reply to Geoffrey Garen from comment #8)
> > Source/WebKit/ChangeLog:48
> > +        No new tests, tested by existing tests.
> 
> Do we have preliminary perf data from this patch?

I added couple of examples. IIRC current ToT MotionMark for me is 800 pts, and this new one is 4500pts and non-pup is 8800 pts.

 
> > Source/WebKit/Platform/IPC/Decoder.h:58
> > +    static constexpr size_t streamMessageAlignment = alignof(uint64_t);
> 
> What are we trying to achieve with this alignment setting? Is the goal to
> enforce a minimum alignment sufficient to read any type?

That's the sort-of-goal of the Encoder/Decoder when encoding a normal message.
A stream message must be encoded with similar restrictions, since if the message does not fit to the stream, it must be sent out of line.
The current Encoder is not able to "stop", so we must convert it on-the-fly to encode a normal message.
The current "coders" e.g. the code using Encoder, are not polymorphic, so we cannot create a StreamEncoder and "normal" Encoder.

It's not ideal.

Added this as a hand-wavy comment in Encoder.h

> > Source/WebKit/Platform/IPC/Decoder.h:59
> > +    static constexpr size_t outOfLineStreamMessageSize = sizeof(uint64_t) * 2;
> 
> Why 2?

This is the message header size. Added this as a comment in the Encoder.h



> > Source/WebKit/Platform/IPC/MessageStreamSender.h:64
> > +            auto wakeupResult = m_stream.commitWrite(Encoder::outOfLineStreamMessageSize);
> 
> Since the StreamMessageOutOfLine message is a flag, why does it need size at
> all? Are we trying to overload the read operation in some way? If so, can we
> do something more explicit instead?

Items in the normal message are:
[message header][message data]

So items in the stream buffer are:
[message header][message data]

"Out of line message" in the stream buffer is:
[message header with OOL bit on]

The reason for the stream message having the message header encoded is explained above: it's not easy currently to encode the message so that the format would be different in stream message than in normal message.

The encoding part is a bit bad, so indeed that might be something to refine.
 
> 
> > Source/WebKit/Platform/IPC/Decoder.h:161
> > -    
> > +
> 
> Whitespace change
> 
> > Source/WebKit/Platform/IPC/DestinationStream.h:39
> > +class DestinationStreamBase {
> 
> I think this might read more clearly as a data member named
> DestinationStream (or just Stream), rather than a base clase. A stream
> reader/writer has a stream to read/write, but is not itself a stream.

Done. (It makes it a bit clunkier in some ways, but might be remedied if the implementation is changed to a different circular buffer scheme, see the FIXME.)


> 
> > Source/WebKit/Platform/IPC/DestinationStream.h:51
> > +        uint64_t cacheLinePadding[2];
> > +        Atomic<size_t> writeOffset;
> 
> I think you can use alignas instead of cacheLinePadding.

So the idea is not to align, if that was what you thought the idea was?
(The idea was that if the two variables are adjacent in memory, they may fall into same cache line. 
I seem to recall this may create (a very slight?) bottleneck: even if the variables are distinct, they are shared from the processors perspective)
or did you mean I could use something like "alignas(uint64_t[2])" ? I did not think of that, I'll see if that can be used..

> 
> > Source/WebKit/Platform/IPC/DestinationStream.h:57
> > +    size_t adjustOffset(size_t offset) {
> 
> This function could use a more specific name. I think the adjustment being
> performed here is wrapping, right?

Changed to wrapOffset.

> 
> > Source/WebKit/Platform/IPC/DestinationStream.h:71
> > +    static constexpr size_t kReadLimitSleepTag = 1 << 31;
> 
> I think this indicates that the reader is sleeping, so I would call it
> kReaderIsSleepingTag instead.
> 
> But also: commitWrite() doesn't seem to consult this tag. Is that a bug?

It does, but in a bit subtle way. The sender knows what it wrote, so it just checks if the value is still there.
I added an comment.

> 
> > Source/WebKit/Platform/IPC/DestinationStream.h:107
> > +    void write(uint8_t*& ptr, size_t& capacity)
> > +    void writep(uint8_t*& ptr, size_t& capacity)

> 
> What's the difference between these two functions?

Debugging leftover, sorry. Removed.

> 
> > Source/WebKit/Platform/IPC/DestinationStream.h:120
> > +        if (readLimit != expectedReadLimit)
> > +            return WakeUpReader::Yes;
> > +        return WakeUpReader::No;
> 
> Should we check kReadLimitSleepTag here?

So it's the "if readLimit is different than what I wrote there previously".
 
> > Source/WebKit/Platform/IPC/MessageStreamReceiver.h:51
> > +            ASSERT(decoder.isValid());
> 
> Probably need a stronger check here.

Done.
Comment 14 Kimmo Kinnunen 2020-12-15 07:10:23 PST
Created attachment 416242 [details]
Patch
Comment 15 Kimmo Kinnunen 2020-12-18 11:21:37 PST
Created attachment 416533 [details]
Patch
Comment 16 Geoffrey Garen 2020-12-18 13:15:51 PST
FYI, as of https://trac.webkit.org/changeset/270938/webkit, you can now use wtf/cocoa/MachSemaphore.h to establish a cross-process semaphore.

I think this means that the sender shouldn't have to live-yield anymore.

Also, maybe the receiver can wait for more data by using a semaphore instead of requiring a new IPC to wake it up (at least in the common case).
Comment 17 Kimmo Kinnunen 2020-12-21 04:40:26 PST
(In reply to Geoffrey Garen from comment #16)
> FYI, as of https://trac.webkit.org/changeset/270938/webkit, you can now use
> wtf/cocoa/MachSemaphore.h to establish a cross-process semaphore.
> 
> I think this means that the sender shouldn't have to live-yield anymore.
> 
> Also, maybe the receiver can wait for more data by using a semaphore instead
> of requiring a new IPC to wake it up (at least in the common case).

Ok.
Comment 18 Kimmo Kinnunen 2021-01-26 06:01:00 PST
Created attachment 418406 [details]
Patch
Comment 19 Kimmo Kinnunen 2021-02-01 13:06:16 PST
Created attachment 418915 [details]
Patch
Comment 20 Kimmo Kinnunen 2021-02-01 13:08:30 PST
(In reply to Geoffrey Garen from comment #16)
> FYI, as of https://trac.webkit.org/changeset/270938/webkit, you can now use
> wtf/cocoa/MachSemaphore.h to establish a cross-process semaphore.
> 
> I think this means that the sender shouldn't have to live-yield anymore.
> 
> Also, maybe the receiver can wait for more data by using a semaphore instead
> of requiring a new IPC to wake it up (at least in the common case).

The new patch addresses this. Unfortunately the ChangeLog is a bit lacking, but I'll work on it more. Just uploading this in case there's interest in giving feedback on the code part.
Comment 21 Kimmo Kinnunen 2021-02-01 13:12:04 PST
Created attachment 418916 [details]
Patch
Comment 22 Geoffrey Garen 2021-02-03 12:59:45 PST
Comment on attachment 418916 [details]
Patch

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

> Source/WebKit/Platform/IPC/ArgumentCoders.cpp:-114
> -

Some stray whitespace changes in this file.

> Source/WebKit/Platform/IPC/Connection.cpp:389
> +    auto messageFilterLocker = holdLock(m_messageFiltersLock);
> +    m_messageFilters.addFilter(messageFilter, receiverName, destinationID);

Would be good to add the message ordering comment here too.

> Source/WebKit/Platform/IPC/Connection.cpp:390
> +    auto locker = holdLock(m_incomingMessagesMutex);

What is the case where we intend to hold m_incomingMessagesMutex without holding m_messageFiltersLock, or vice versa? Maybe we can reduce to just one lock?

> Source/WebKit/Platform/IPC/Connection.cpp:396
> +            m_incomingMessages.remove(it);
> +            end = m_incomingMessages.end();

I think this is a bug. remove() invalidates 'it', so re-reading end() here is not sufficient.

Also, it's kind of expensive to do O(n) removal each time through the loop.

Also, I think it would be better to use an explicit return value to indicate message consumption, rather than testing for null. C++ does not require that moved-from values be null.

> Source/WebKit/Platform/IPC/Connection.cpp:447
> -    
> +

Some whitespace changes in this file.

> Source/WebKit/Platform/IPC/Connection.cpp:1154
> +    // FIXME: This has a message ordering bug where the early messages dispatched here would arrive
> +    // later than later messages that get dispatched by the message processing queue. This should be fixed by making WorkQueueReceiver
> +    // a message filter.

Do we need to fix this before landing? (Seems like probably?)
Comment 23 Kimmo Kinnunen 2021-02-04 11:10:55 PST
Thanks for the review.

I'll fix the whitespace and move some commits away from the patch.


(In reply to Geoffrey Garen from comment #22)
> Comment on attachment 418916 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=418916&action=review
> 
> > Source/WebKit/Platform/IPC/ArgumentCoders.cpp:-114
> > -
> 
> Some stray whitespace changes in this file.
> 
> > Source/WebKit/Platform/IPC/Connection.cpp:389
> > +    auto messageFilterLocker = holdLock(m_messageFiltersLock);
> > +    m_messageFilters.addFilter(messageFilter, receiverName, destinationID);
> 
> Would be good to add the message ordering comment here too.
> 
> > Source/WebKit/Platform/IPC/Connection.cpp:390
> > +    auto locker = holdLock(m_incomingMessagesMutex);
> 
> What is the case where we intend to hold m_incomingMessagesMutex without
> holding m_messageFiltersLock, or vice versa? Maybe we can reduce to just one
> lock?
> 
> > Source/WebKit/Platform/IPC/Connection.cpp:396
> > +            m_incomingMessages.remove(it);
> > +            end = m_incomingMessages.end();
> 
> I think this is a bug. remove() invalidates 'it', so re-reading end() here
> is not sufficient.

Right, I'll do it another way. As far as I understood looking at the implementation it didn't invalidate the iterator but yes, it's sketchy.

> Also, it's kind of expensive to do O(n) removal each time through the loop.
 
Right, I'll do it another way.

> Also, I think it would be better to use an explicit return value to indicate
> message consumption, rather than testing for null. C++ does not require that
> moved-from values be null.

Sure. I'll change the contract..
FWIW, C++ does require moved-away unique_ptr to be null, which is being used here.

https://en.cppreference.com/w/cpp/memory/unique_ptr/unique_ptr

  unique_ptr( unique_ptr&& u ) noexcept; (5)

  5) Constructs a unique_ptr by transferring ownership from u to *this and stores the.  null pointer in u. 


> > Source/WebKit/Platform/IPC/Connection.cpp:447
> > -    
> > +
> 
> Some whitespace changes in this file.
> 
> > Source/WebKit/Platform/IPC/Connection.cpp:1154
> > +    // FIXME: This has a message ordering bug where the early messages dispatched here would arrive
> > +    // later than later messages that get dispatched by the message processing queue. This should be fixed by making WorkQueueReceiver
> > +    // a message filter.
> 
> Do we need to fix this before landing? (Seems like probably?)

I can do the fix when moving the code you commented to another patch, but it's a bit peripheral. For this patch it's not needed.
Comment 24 Kimmo Kinnunen 2021-02-05 10:29:20 PST
Created attachment 419427 [details]
rebased to tot, not fully split
Comment 25 Kimmo Kinnunen 2021-02-09 05:44:59 PST
Created attachment 419705 [details]
removed ipc message receive queue
Comment 26 Kimmo Kinnunen 2021-02-09 05:51:04 PST
Moved the code commented below into bug 221560.

(In reply to Geoffrey Garen from comment #22)
> Some stray whitespace changes in this file.

Done.

> What is the case where we intend to hold m_incomingMessagesMutex without
> holding m_messageFiltersLock, or vice versa? Maybe we can reduce to just one
> lock?

Done in other patch, uses m_incomingMessagesMutex.
 
> I think this is a bug. remove() invalidates 'it', so re-reading end() here
> is not sufficient.
> Also, it's kind of expensive to do O(n) removal each time through the loop.
 
Done in the other patch. It willl construct a new list with the remaining items and swap.

 
> Also, I think it would be better to use an explicit return value to indicate
> message consumption, rather than testing for null.

Done in other patch. The API now always transfers the decoder to the client, no programmatic opt out.

> Some whitespace changes in this file.

Done

> Do we need to fix this before landing? (Seems like probably?)

Done in the other patch. Only few examples, not all call sites.
Comment 27 Kimmo Kinnunen 2021-02-09 06:33:06 PST
Created attachment 419710 [details]
ready for review
Comment 28 Myles C. Maxfield 2021-02-12 09:04:53 PST
(In reply to Kimmo Kinnunen from comment #26)
> Moved the code commented below into bug 221560.
> 
> (In reply to Geoffrey Garen from comment #22)
> > Some stray whitespace changes in this file.
> 
> Done.

Comments like these are fine, but not necessary.
Comment 29 Kimmo Kinnunen 2021-02-17 01:01:34 PST
Created attachment 420612 [details]
Patch
Comment 30 Kimmo Kinnunen 2021-02-17 02:27:53 PST
Created attachment 420623 [details]
Patch
Comment 31 Kimmo Kinnunen 2021-02-17 03:34:02 PST
Created attachment 420625 [details]
Patch
Comment 32 Kimmo Kinnunen 2021-02-17 03:47:47 PST
Created attachment 420627 [details]
Patch
Comment 33 Kimmo Kinnunen 2021-02-17 10:59:01 PST
Created attachment 420671 [details]
Patch
Comment 34 Kimmo Kinnunen 2021-02-17 11:14:49 PST
Created attachment 420677 [details]
Patch
Comment 35 Kimmo Kinnunen 2021-02-17 11:26:37 PST
Created attachment 420684 [details]
Patch
Comment 36 Kimmo Kinnunen 2021-02-17 11:46:09 PST
Created attachment 420687 [details]
Patch
Comment 37 Kimmo Kinnunen 2021-02-18 00:11:16 PST
Created attachment 420802 [details]
Patch
Comment 38 Kimmo Kinnunen 2021-02-18 06:00:12 PST
Created attachment 420824 [details]
Patch
Comment 39 Geoffrey Garen 2021-02-18 16:34:05 PST
Comment on attachment 420824 [details]
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.
Comment 40 Geoffrey Garen 2021-02-18 16:34:55 PST
> Still, I'd suggest re-uploading and hitting cq+ to make sure this lands cleanly.

... since that will get the benefit of an up-to-date EWS run.
Comment 41 Kimmo Kinnunen 2021-02-19 01:40:35 PST
Created attachment 420941 [details]
Patch
Comment 42 Kimmo Kinnunen 2021-02-19 04:41:13 PST
Created attachment 420956 [details]
Patch
Comment 43 Kimmo Kinnunen 2021-02-19 06:01:59 PST
Created attachment 420959 [details]
Patch
Comment 44 Kimmo Kinnunen 2021-02-19 22:22:47 PST
Created attachment 421080 [details]
Patch
Comment 45 Kimmo Kinnunen 2021-02-20 12:20:10 PST
Created attachment 421097 [details]
Patch for landing
Comment 46 Peng Liu 2021-02-20 12:35:23 PST
Comment on attachment 421097 [details]
Patch for landing

Clearing flags on attachment: 421097

Committed r273204 (234390@main): <https://commits.webkit.org/234390@main>
Comment 47 Peng Liu 2021-02-20 12:35:28 PST
All reviewed patches have been landed.  Closing bug.