[webkit-reviews] review granted: [Bug 194665] [Web GPU] Buffer updates part 1: async mapping functions, unmap, and destroy : [Attachment 362792] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Justin Fan <justin_fan at apple.com>'s
request for review:
Bug 194665: [Web GPU] Buffer updates part 1: async mapping functions, unmap,
and destroy
https://bugs.webkit.org/show_bug.cgi?id=194665

Attachment 362792: Patch

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




--- Comment #24 from Dean Jackson <dino at apple.com> ---
Comment on attachment 362792
  --> https://bugs.webkit.org/attachment.cgi?id=362792
Patch

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

> Source/WebCore/Modules/webgpu/WebGPUBindGroupDescriptor.cpp:42
> +#if LOG_DISABLED
> +    UNUSED_PARAM(functionName);
> +#endif

You probably don't really need the #if here because the compiler will optimise
away the void statement. But whatever.

> Source/WebCore/Modules/webgpu/WebGPUBindGroupDescriptor.cpp:104
> +	       if (view)
> +		   return static_cast<GPUBindingResource>(view->texture());
> +	       return WTF::nullopt;
> +	   }, [&layoutBinding, functionName] (const WebGPUBufferBinding&
binding) -> Optional<GPUBindingResource> {
> +	       if (binding.buffer && binding.buffer->buffer()) {
> +		   if
(!validateBufferBindingType(binding.buffer->buffer().get(), layoutBinding,
functionName))
> +		       return WTF::nullopt;
> +
> +		   return static_cast<GPUBindingResource>(GPUBufferBinding {
binding.buffer->buffer().releaseNonNull(), binding.offset, binding.size });
> +	       }
> +	       LOG(WebGPU, "%s: Invalid GPUBindingResource for binding %lu in
GPUBindGroupBindings!", functionName, layoutBinding.binding);
> +	       return WTF::nullopt;

I suggest flipping the logic in these two functions so the error case comes
first. e.g.

if (!view)
  return WTF::nullopt;

return static_cast<GPUBindingResource>(view->texture());

> Source/WebCore/Modules/webgpu/WebGPUBindGroupDescriptor.cpp:111
> +	   if (bindingResource)
> +	       bindGroupBindings.uncheckedAppend(GPUBindGroupBinding {
binding.binding, WTFMove(bindingResource.value()) });
> +	   else
> +	       return WTF::nullopt;

Same here.

> Source/WebCore/Modules/webgpu/WebGPUBuffer.cpp:87
> +    m_buffer->registerMappingCallback([promise = WTFMove(promise)]
(JSC::ArrayBuffer* arrayBuffer) mutable {
> +	   if (!arrayBuffer) {
> +	       promise.reject();
> +	       return;
> +	   }
> +
> +	   promise.resolve(arrayBuffer);
> +    }, isRead);

Question: Since we are not keeping a reference to the promise anywhere but in
this closure, can we guarantee that this callback will always be called? i.e.
if we call a combination of mapReadAsync mapWriteAsync unmap destroy, will the
buffer always callback?

> Source/WebCore/Modules/webgpu/WebGPUBuffer.h:44
> +using ArrayBufferPromise =
DOMPromiseDeferred<IDLInterface<JSC::ArrayBuffer*>>;

Suggestion: maybe this should be BufferReadOrWritePromise? It's not a promise
on the ArrayBuffer.
Looking through WebCore, most cases like this are named FooPromise, with Foo
being related to the call that takes the promise.

Also, should the using be inside the class? I think it should.

> Source/WebCore/platform/graphics/gpu/cocoa/GPUBufferMetal.mm:119
> +    ASSERT(!m_pendingCallback && !m_mappingCallbackTask.hasPendingTask());
> +

I think we should check here if the m_pendingCallback exists, and if it
does.... hmmm... reject it and the new one?

Or does the logic above already handle this? Should we also have a generic test
for isMappable()?

> Source/WebCore/platform/graphics/gpu/cocoa/GPUBufferMetal.mm:131
> +	       if (m_isMapReadable)
> +		   m_pendingCallback->callback(stagingBufferForRead());
> +	       else
> +		   m_pendingCallback->callback(stagingBufferForWrite());

How about a ternary here?

m_pendingCallback->callback(m_isMapReadable ? stagingBufferForRead() :
stagingBufferForWrite());

> LayoutTests/webgpu/map-write-buffers.html:70
> +	   const promise = buffer.mapWriteAsync();
> +	   buffer.unmap();
> +
> +	   await promise.then(() => {

Don't we have to check if the promise has been resolved instantly? Or does
.then() still execute in that case?