[webkit-reviews] review granted: [Bug 218426] [Concurrent display lists] Add an initial implementation of concurrent display list rendering : [Attachment 413729] Use CheckedSize instead of Checked<>

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 218426: [Concurrent display lists] Add an initial implementation of
concurrent display list rendering
https://bugs.webkit.org/show_bug.cgi?id=218426

Attachment 413729: Use CheckedSize instead of Checked<>

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




--- Comment #15 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 413729
  --> https://bugs.webkit.org/attachment.cgi?id=413729
Use CheckedSize instead of Checked<>

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

> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:136
> +	   auto locker = SharedDisplayListHandle::Lock { handle };
> +	   sizeToRead = handle.unreadBytes();

This seems error prone. Can't we just add the lock to unreadBytes() instead?

> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:167
> +	   auto locker = SharedDisplayListHandle::Lock { handle };
> +	   handle.advance(sizeToRead);
> +	   sizeToRead = handle.unreadBytes();

Ditto about moving the lock into advance. Maybe advance can return the new
unread bytes value.

> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:181
> +    auto initialHandle = m_sharedDisplayListHandles.get(initialIdentifier);
> +    if (UNLIKELY(!initialHandle)) {

As we discussed, it's a bit dangerous to rely on the fact the map won't be
mutated while we're accessing this handle.
We should either make the handle ref-counted or add some kind of message check
to make sure we don't remove stuff from the map.

> Source/WebKit/Shared/SharedDisplayListHandle.h:57
> +		   std::this_thread::yield();

Why are we yielding before checking the value at least once? That seems rather
inefficient.
Also, use Thread::yield?

> Source/WebKit/Shared/SharedDisplayListHandle.h:61
> +	       }

We should probably limit ourselves from spinning forever.
We probably need an equivalent of parking lot cross-process.
e.g. Use the OS-level lock feature for example.
We can do that in a follow up patch though.

> Source/WebKit/Shared/SharedDisplayListHandle.h:83
> +    struct DisplayListHandleStructure {

I would have called this DisplayListSharedMemoryHeader or something.
"Structure" sounds rather vague & sounds like something related to
JSC::Structure, which it is not.

> Source/WebKit/Shared/SharedDisplayListHandle.h:88
> +    std::atomic<uint64_t>& atomicLockValue() { return structure().lock; }

Maybe use WTF::Atomic instead?

> Source/WebKit/WebProcess/GPU/graphics/RemoteRenderingBackendProxy.cpp:132
> +	   auto locker = SharedDisplayListHandle::Lock { *sharedHandle };
> +
> +	   bool unreadCountWasEmpty = !sharedHandle->unreadBytes();
> +	   sharedHandle->advance(handle.capacity);

It seems like this is the only part we need to put under lock?
Can we move the lock into advance? Maybe advance can return the old & new
values of unread bytes.