Bug 191911 – Implement WebGPUQueue and device.getQueue()

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dean Jackson]

Implement WebGPUQueue and device.getQueue()

[Radar WebKit Bug Importer]

<rdar://problem/46214871>

[Dean Jackson]

Created attachment 355478 [details]
Patch

[Antoine Quint]

Comment on attachment 355478 [details]
Patch

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

> Source/WebCore/Modules/webgpu/WebGPUQueue.cpp:54
> +        gpuBuffers.append(buffer->commandBuffer());

This could use Vector::map().

> Source/WebCore/Modules/webgpu/WebGPUQueue.idl:30
> +    ImplementationLacksVTable

Is this necessary?

> Source/WebCore/platform/graphics/gpu/cocoa/GPUQueueMetal.mm:59
> +        LOG(WebGPU, "GPUQueue::create(): Unable to create MTLCommandQueue.");

So much better.

> LayoutTests/webgpu/queue-creation.html:15
> +    const swapChain = canvas.getContext("webgpu");

Remove the canvas variable and do it in a single statement.

> LayoutTests/webgpu/queue-creation.html:29
> +}, "getQueue() on WebGPUDevice");

I would put a full stop at the end of all those assertions.

[Dean Jackson]

Created attachment 355482 [details]
Patch

[Dean Jackson]

Created attachment 355483 [details]
Patch

[Dean Jackson]

Committed r238451: <https://trac.webkit.org/changeset/238451>

[Sam Weinig]

Comment on attachment 355478 [details]
Patch

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

Sorry for the post landing review.

> Source/WebCore/Modules/webgpu/WebGPUDevice.idl:42
>      WebGPUQueue getQueue();

Should this actually return a WebGPUQueue? since it can return null in some circumstances (I don't think this would actually effect code gen right now, but might in the future).

>> Source/WebCore/Modules/webgpu/WebGPUQueue.cpp:54
>> +    Vector<Ref<const GPUCommandBuffer>> gpuBuffers { buffers.size() };
>> +    for (const auto& buffer : buffers)
>> +        gpuBuffers.append(buffer->commandBuffer());
> 
> This could use Vector::map().

This way of initialing gpuBuffers is a little less efficient than it could be. Instead, the preferred way is:

Vector<Ref<const GPUCommandBuffer>> gpuBuffers;
gpuBuffers.reserveInitialCapacity(buffers.size());
for (const auto& buffer : buffers)
    gpuBuffers.appendUnchecked(buffer->commandBuffer());

This avoids unnecessary double construction of all the Ref<const GPUCommandBuffer>s in gpuBuffers (by doing setting the capacity rather than the size), and appendUnchecked avoids a few capacity checks.

> LayoutTests/webgpu/queue-creation.html:27
> +    queue.label = "Example label";

Might be good to test what the default label is before setting it.

[Tim Horton]

This test is failing a lot in EWS (see https://webkit-queues.webkit.org/results/10135967 for example)...

[Ryan Haddad]

(In reply to Tim Horton from comment #8)
> This test is failing a lot in EWS (see
> https://webkit-queues.webkit.org/results/10135967 for example)...
--- /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/webgpu/queue-creation-expected.txt
+++ /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/webgpu/queue-creation-actual.txt
@@ -1,3 +1,3 @@
 
-PASS getQueue() on WebGPUDevice 
+PASS getQueue() on WebGPUDevice.

[Ryan Haddad]

(In reply to Ryan Haddad from comment #9)
> (In reply to Tim Horton from comment #8)

I went ahead and just rebaselined the test in https://trac.webkit.org/changeset/238487/webkit