[webkit-reviews] review denied: [Bug 219641] WebGL GPU process IPC should be faster than current WebKit IPC : [Attachment 415993] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Kimmo Kinnunen
<kkinnunen at apple.com>'s request for review:
Bug 219641: WebGL GPU process IPC should be faster than current WebKit IPC
https://bugs.webkit.org/show_bug.cgi?id=219641

Attachment 415993: Patch

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




--- Comment #8 from Geoffrey Garen <ggaren at apple.com> ---
Comment on attachment 415993
  --> https://bugs.webkit.org/attachment.cgi?id=415993
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?