[webkit-reviews] review canceled: [Bug 218426] [Concurrent display lists] Add an initial implementation of concurrent display list rendering : [Attachment 413534] Depends on #218689

[bugzilla-daemon at webkit.org]

Wenson Hsieh <wenson_hsieh at apple.com> has canceled 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 413534: Depends on #218689

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




--- Comment #12 from Wenson Hsieh <wenson_hsieh at apple.com> ---
Comment on attachment 413534
  --> https://bugs.webkit.org/attachment.cgi?id=413534
Depends on #218689

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

>> Source/WebKit/GPUProcess/graphics/DisplayListReaderHandle.cpp:34
>> +	unreadBytesHandle() -= amount;
> 
> Check for underflow?

Done! Made this robust against underflow.

>> Source/WebKit/GPUProcess/graphics/DisplayListReaderHandle.h:42
>> +	std::unique_ptr<WebCore::DisplayList::DisplayList>
createDisplayList(size_t offset, size_t capacity,
WebCore::DisplayList::ItemBufferReadingClient&) const;
> 
> Maybe readDisplayListFromHandle() to not make it sound like it creates a
fresh, empty DisplayList.

I do agree `readDisplayListFromHandle` is better than my current name, but at
the same time `readDisplayListFromHandle` sounds like it's performing an action
(namely, reading a display list). I'll change this to
`displayListForReadingFromHandle`.

>> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:128
>> +void
RemoteRenderingBackend::wakeUpAndProcessDisplayList(WebCore::DisplayList::ItemB
ufferIdentifier initialIdentifier, uint64_t initialOffset,
RenderingResourceIdentifier renderingResourceIdentifier)
> 
> renderingResourceIdentifier -> destinationBufferIdentifier

Changed to destinationBufferIdentifier.

>> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:130
>> +	auto imageBuffer =
m_remoteResourceCache.cachedImageBuffer(renderingResourceIdentifier);
> 
> Why is this getting a designation buffer from the source cache? Resouce cache
buffers are readonly in the GPU process.

The current status quo is that remote image buffers that back canvases are
tracked in the GPU process as cached resources (see
RemoteRenderingBackend::createImageBuffer, above). I agree it's a bit
confusing, but this might have benefits (e.g. when painting from canvas to
canvas). Perhaps RemoteResourceCache and its cacheImageBuffer method should be
renamed to something more general? (For example,
RemoteResourceStore::addImageBuffer or
RemoteResourceRegistry::registerImageBuffer).

>> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:132
>>	    // FIXME: Add a message check to terminate the web process.
> 
> I think we need to do this very soon.

Yes!

>> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:146
>> +	    SharedDisplayListHandle::Lock locker { *sharedHandle };
> 
> The cool kids do something like auto locker = SharedDisplayListHandle::Lock {
... }

����

>> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:151
>> +	   
imageBuffer->flushDisplayList(*sharedHandle->createDisplayList(offset,
sizeToRead, *this));
> 
> We need to call this something other than "flushDisplayList". It's more
"render display list".

`renderDisplayList` sounds good to me.

I think I'll split this renaming out into a different patch.

>> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:153
>> +	    offset += sizeToRead;
> 
> Do we need overflow checks?

Added!

>> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:166
>> +	    sharedHandle = m_sharedDisplayListHandles.get(nextIdentifier);
> 
> I think I'd feel more comfortable if you didn't change the sharedHandle in
this loop, since sharedHandle and sizeToRead go together and it would be easy
to introduce mistakes in future. Can you refractor the code to read from a
single handle into its own function?

Yeah, that makes sense — I will split out the logic for reading from a single
shared handle into a new private helper.

>> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:203
>> +void
RemoteRenderingBackend::didCreateSharedDisplayListHandle(DisplayList::ItemBuffe
rIdentifier identifier, const SharedMemory::IPCHandle& handle,
RenderingResourceIdentifier remoteResourceIdentifier)
> 
> remoteResourceIdentifier -> destinationBufferIdentifier

Fixed!

>> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:205
>> +	if (auto sharedMemory = SharedMemory::map(handle.handle,
SharedMemory::Protection::ReadWrite))
> 
> Why are we mapping ReadWrite in the GPUP? Is this just to be able to write
unreadBytes? Should that instead be in its own little shared memory handle?

Yes, it is so that we can update the "unread bytes" count. I recall discussing
it with Geoff, and he suggested that we should just stick it in the first bit
of each shared memory chunk instead of taking up a whole new page for an
atomic.

>> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.h:105
>> +	void
didCreateSharedDisplayListHandle(WebCore::DisplayList::ItemBufferIdentifier,
const SharedMemory::IPCHandle&, WebCore::RenderingResourceIdentifier);
> 
> Give RenderingResourceIdentifier a name

���� "destinationBufferIdentifier"

>> Source/WebKit/Shared/SharedDisplayListHandle.h:38
>> +	static constexpr auto reservedCapacityAtStart = sizeof(bool) +
sizeof(size_t);
> 
> Maybe round this up for better alignment?

Done! (changed to 16 bytes)

>> Source/WebKit/Shared/SharedDisplayListHandle.h:58
>> +			break;
> 
> Please check with a locking/scheduling/priority-donating expert.

I came up with this part with some help from Geoff; perhaps he has some more
thoughts on this!

One thing we should investigate is changing this to use a single atomic size_t
counter instead of an atomic lock. When I tried doing this, however, it ended
up hurting performance for (as-of-yet) unknown reasons...

>> Source/WebKit/WebProcess/GPU/graphics/DisplayListWriterHandle.cpp:35
>> +	unreadBytesHandle() += amount;
> 
> Check for overflow?

Fixed!

>> Source/WebKit/WebProcess/GPU/graphics/DisplayListWriterHandle.cpp:40
>> +	return sharedMemory().size() - writableOffset();
> 
> Check for underflow?

Fixed!

>> Source/WebKit/WebProcess/GPU/graphics/DisplayListWriterHandle.cpp:55
>> +	    ASSERT(m_writableOffset ==
SharedDisplayListHandle::reservedCapacityAtStart);
> 
> RELEASE_ASSERT?

Changed to RELEASE_ASSERT.

>> Source/WebKit/WebProcess/GPU/graphics/DisplayListWriterHandle.cpp:59
>> +	Lock locker { *this };
> 
> auto locker =

Fixed!

>> Source/WebKit/WebProcess/GPU/graphics/RemoteRenderingBackendProxy.cpp:129
>> +	    SharedDisplayListHandle::Lock locker { *sharedHandle };
> 
> auto locker =

Fixed!