[webkit-reviews] review granted: [Bug 223732] [GPU Process]: Improve getImageData() perf part 2: Use shared memory and a semaphore : [Attachment 424305] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 223732: [GPU Process]: Improve getImageData() perf part 2: Use shared
memory and a semaphore
https://bugs.webkit.org/show_bug.cgi?id=223732

Attachment 424305: Patch

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




--- Comment #8 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 424305
  --> https://bugs.webkit.org/attachment.cgi?id=424305
Patch

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

> Source/WebKit/GPUProcess/graphics/RemoteImageBuffer.h:82
> +	       auto getImageDataSharedMemory =
m_remoteRenderingBackend.getImageDataSharedMemory();

Maybe just sharedMemory is sufficient since it's pretty obvious we're dealing
with GetImageData here.

> Source/WebKit/GPUProcess/graphics/RemoteImageBuffer.h:86
> +	       else {
> +		   if (getImageDataSharedMemory)

Why not else if?

> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:328
> +    ASSERT(!m_getImageDataSharedMemory || byteCount >
m_getImageDataSharedMemory->size());
> +    m_getImageDataSharedMemory = SharedMemory::allocate(byteCount);

We should probably limit byteCount at some size, say, 32MB.

> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.h:82
> +    RefPtr<SharedMemory> getImageDataSharedMemory() { return
m_getImageDataSharedMemory; }

Maybe sharedMemoryForGetImageData?

> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.h:121
> +    void sharedMemoryForGetImageData(size_t byteCount,
CompletionHandler<void(const SharedMemory::IPCHandle&)>&&);

Maybe updateSharedMemoryForGetImageData?
The crucial thing here is that we want the new size, right?

> Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferProxy.h:201
> +	   SharedMemory* sharedMemory =
m_remoteRenderingBackendProxy->maybePrepareSharedMemoryForGetImageData(dataSize
);

Why not just sharedMemoryForGetImageData?