[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 bugzilla-daemon at webkit.org
Mon Nov 9 11:48:54 PST 2020

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

Attachment 413534: Depends on #218689


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

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

Changed to destinationBufferIdentifier.

>> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:130
>> +	auto imageBuffer =
> 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

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


>> 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
>> +	   
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?


>> 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
rIdentifier identifier, const SharedMemory::IPCHandle& handle,
RenderingResourceIdentifier remoteResourceIdentifier)
> remoteResourceIdentifier -> destinationBufferIdentifier


>> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:205
>> +	if (auto sharedMemory = SharedMemory::map(handle.handle,
> 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

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

���� "destinationBufferIdentifier"

>> Source/WebKit/Shared/SharedDisplayListHandle.h:38
>> +	static constexpr auto reservedCapacityAtStart = sizeof(bool) +
> 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?


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


>> Source/WebKit/WebProcess/GPU/graphics/DisplayListWriterHandle.cpp:55
>> +	    ASSERT(m_writableOffset ==


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


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


More information about the webkit-reviews mailing list