[webkit-reviews] review granted: [Bug 187333] Improve WebGPU implementation, including using Metal Objective-C protocols more simply and correctly : [Attachment 344307] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has granted Darin Adler <darin at apple.com>'s request
for review:
Bug 187333: Improve WebGPU implementation, including using Metal Objective-C
protocols more simply and correctly
https://bugs.webkit.org/show_bug.cgi?id=187333

Attachment 344307: Patch

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




--- Comment #15 from Sam Weinig <sam at webkit.org> ---
Comment on attachment 344307
  --> https://bugs.webkit.org/attachment.cgi?id=344307
Patch

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

> Source/WebCore/ChangeLog:74
> +	   (WebCore::WebGPUBuffer::create): Return nullptr if we fail to create
a GPU buffer.
> +	   Also use more references rather than pointers for things that are
never null.

This is a change in behavior (we always used to return an object). What is
motivating this change? Do we have a test for this new behavior?

> Source/WebCore/ChangeLog:268
> +	   * platform/graphics/cocoa/GPUBufferMetal.mm:

Instead of being in the platform/graphics/cocoa directory, should we move this
(and all the other metal files, to a platform/graphics/metal directory?

> Source/WTF/wtf/Compiler.h:303
> +#if !defined(OBJC_PROTOCOL) && defined(__OBJC__)
> +#define OBJC_PROTOCOL(protocolName) @protocol protocolName; using
protocolName = NSObject <protocolName>
> +#endif

I'm not clear on what the 'using...' part is doing. Might be worth a comment.

> Source/WebCore/html/canvas/WebGPUCommandBuffer.cpp:75
>  RefPtr<WebGPURenderCommandEncoder>
WebGPUCommandBuffer::createRenderCommandEncoderWithDescriptor(WebGPURenderPassD
escriptor& descriptor)
>  {
> -    RefPtr<WebGPURenderCommandEncoder> commandEncoder =
WebGPURenderCommandEncoder::create(this->context(), this, &descriptor);
> -    return commandEncoder;
> +    return WebGPURenderCommandEncoder::create(*context(), m_buffer,
descriptor.descriptor());
>  }

It looks like WebGPURenderCommandEncoder::create returns a
Ref<WebGPURenderCommandEncoder>, so this function could as well.

> Source/WebCore/html/canvas/WebGPUCommandBuffer.cpp:80
>  RefPtr<WebGPUComputeCommandEncoder>
WebGPUCommandBuffer::createComputeCommandEncoder()
>  {
> -    RefPtr<WebGPUComputeCommandEncoder> commandEncoder =
WebGPUComputeCommandEncoder::create(this->context(), this);
> -    return commandEncoder;
> +    return WebGPUComputeCommandEncoder::create(*context(), m_buffer);
>  }

Ditto.

> Source/WebCore/html/canvas/WebGPUCommandQueue.cpp:52
>  RefPtr<WebGPUCommandBuffer> WebGPUCommandQueue::createCommandBuffer()
>  {
> -    RefPtr<WebGPUCommandBuffer> buffer =
WebGPUCommandBuffer::create(this->context(), this);
> -    return buffer;
> +    return WebGPUCommandBuffer::create(*context(), m_queue);
>  }

WebGPUCommandBuffer::create returns a Ref<WebGPUCommandBuffer>, so this
function can as well.

> Source/WebCore/html/canvas/WebGPUDrawable.cpp:45
> +    , m_texture { WebGPUTexture::createFromDrawableTexture(context,
GPUTexture { m_drawable }) }

WebGPUTexture::createFromDrawableTexture returns a Ref<WebGPUTexture>, so
m_texture can be a Ref<WebGPUTexture>.

> Source/WebCore/html/canvas/WebGPULibrary.cpp:58
> +    auto function = WebGPUFunction::create(*context(), m_library, name);
>      if (!function->function())
>	   return nullptr;
> -    return function;
> +    return WTFMove(function);

It seems like this could be done slightly more efficiently if the GPUFunction
was created first, checked to see if the function was non-null, and then only
if it was non-null, moved into a newly created WebGPUFunction. This would avoid
the allocation of the WebGPUFunction in the case that the function doesn't
exist.

> Source/WebCore/html/canvas/WebGPURenderPassDescriptor.cpp:47
> -RefPtr<WebGPURenderPassDepthAttachmentDescriptor>
WebGPURenderPassDescriptor::depthAttachment()
> +WebGPURenderPassDepthAttachmentDescriptor*
WebGPURenderPassDescriptor::depthAttachment()
>  {
> -    if (!m_renderPassDescriptor)
> -	   return nullptr;
> -
> -    if (!m_depthAttachmentDescriptor) {
> -	   RefPtr<GPURenderPassDepthAttachmentDescriptor>
platformDepthAttachment = m_renderPassDescriptor->depthAttachment();
> -	   m_depthAttachmentDescriptor =
WebGPURenderPassDepthAttachmentDescriptor::create(this->context(),
platformDepthAttachment.get());
> -    }
> -    return m_depthAttachmentDescriptor;
> +    if (!m_depthAttachment)
> +	   m_depthAttachment =
WebGPURenderPassDepthAttachmentDescriptor::create(*context(),
m_descriptor.depthAttachment());
> +    return m_depthAttachment.get();
>  }

WebGPURenderPassDepthAttachmentDescriptor::create returns a
Ref<WebGPURenderPassDepthAttachmentDescriptor>, so I think this function can
also return a Ref<> (or a reference).

> Source/WebCore/html/canvas/WebGPURenderingContext.cpp:82
> -    auto renderingContext = std::unique_ptr<WebGPURenderingContext>(new
WebGPURenderingContext(canvas, device.releaseNonNull()));
> +    auto renderingContext = std::unique_ptr<WebGPURenderingContext>(new
WebGPURenderingContext(canvas, WTFMove(device)));

Can std::make_unique be used here?

> Source/WebCore/html/canvas/WebGPURenderingContext.cpp:162
>  RefPtr<WebGPURenderPipelineState>
WebGPURenderingContext::createRenderPipelineState(WebGPURenderPipelineDescripto
r& descriptor)
>  {
> -    RefPtr<WebGPURenderPipelineState> state =
WebGPURenderPipelineState::create(this, &descriptor);
> -    return state;
> +    return WebGPURenderPipelineState::create(*this,
descriptor.descriptor());
>  }

This can return a Ref<WebGPURenderPipelineState>.

> Source/WebCore/html/canvas/WebGPURenderingContext.cpp:167
>  RefPtr<WebGPUDepthStencilState>
WebGPURenderingContext::createDepthStencilState(WebGPUDepthStencilDescriptor&
descriptor)
>  {
> -    RefPtr<WebGPUDepthStencilState> state =
WebGPUDepthStencilState::create(this, &descriptor);
> -    return state;
> +    return WebGPUDepthStencilState::create(*this, descriptor.descriptor());
>  }

This can return a Ref<WebGPURenderPipelineState>

> Source/WebCore/html/canvas/WebGPURenderingContext.cpp:172
>  RefPtr<WebGPUComputePipelineState>
WebGPURenderingContext::createComputePipelineState(WebGPUFunction& function)
>  {
> -    RefPtr<WebGPUComputePipelineState> state =
WebGPUComputePipelineState::create(this, &function);
> -    return state;
> +    return WebGPUComputePipelineState::create(*this, function.function());
>  }

This can return a Ref<WebGPURenderPipelineState>.

> Source/WebCore/html/canvas/WebGPURenderingContext.cpp:177
>  RefPtr<WebGPUCommandQueue> WebGPURenderingContext::createCommandQueue()
>  {
> -    RefPtr<WebGPUCommandQueue> queue = WebGPUCommandQueue::create(this);
> -    return queue;
> +    return WebGPUCommandQueue::create(*this);
>  }

This can return a Ref<WebGPUCommandQueue>.

> Source/WebCore/html/canvas/WebGPURenderingContext.cpp:182
>  RefPtr<WebGPUDrawable> WebGPURenderingContext::nextDrawable()
>  {
> -    RefPtr<WebGPUDrawable> drawable = WebGPUDrawable::create(this);
> -    return drawable;
> +    return WebGPUDrawable::create(*this);
>  }

This can return a Ref<WebGPUDrawable>

> Source/WebCore/html/canvas/WebGPURenderingContext.cpp:192
>  RefPtr<WebGPUTexture>
WebGPURenderingContext::createTexture(WebGPUTextureDescriptor& descriptor)
>  {
> -    RefPtr<WebGPUTexture> texture = WebGPUTexture::create(this,
&descriptor);
> -    return texture;
> +    return WebGPUTexture::create(*this, descriptor.descriptor());
>  }

This can return a Ref<WebGPUTexture>.

> Source/WebCore/platform/graphics/cocoa/GPURenderPipelineDescriptorMetal.mm:59
> +    if (!newFunction.metal())
> +	   return;
> +    [m_metal setVertexFunction:newFunction.metal()];

This is changing behavior when newFunction.metal() is nil, it used to call
setVertexFunction with nil. Why is that the right change?

> Source/WebCore/platform/graphics/cocoa/GPURenderPipelineDescriptorMetal.mm:66
> +    if (!newFunction.metal())
> +	   return;
> +    [m_metal setFragmentFunction:newFunction.metal()];

Ditto.

> Source/WebCore/platform/graphics/cocoa/GPURenderPipelineStateMetal.mm:40
> +GPURenderPipelineState::GPURenderPipelineState(const GPUDevice& device,
const GPURenderPipelineDescriptor& descriptor)
> +    : m_metal { adoptNS([device.metal()
newRenderPipelineStateWithDescriptor:descriptor.metal() error:nil]) }

Should we be checking this error value?

> Source/WebCore/platform/graphics/gpu/GPUCommandBuffer.cpp:55
> +DOMPromiseProxy<IDLVoid>& GPUCommandBuffer::completed()
>  {
> +    if (!m_addedCompletedHandler) {
> +	   addCompletedHandler();
> +	   m_addedCompletedHandler = true;
> +    }
>      return m_completedPromise;
>  }

This feels like a layering violation. Classes in the platform layer should not
be using DOMPromiseProxy.  I believe what we have done traditionally is just
use stored callback functions.