Bug 195077 – [Web GPU] Buffer updates part 2: setSubData, GPU/CPU synchronization

• Component: WebGPU
• Status: RESOLVED FIXED

[Justin Fan]

[Web GPU] Buffer updates part 2: setSubData, GPU/CPU synchronization

[Justin Fan]

<rdar://problem/47805229>

[Justin Fan]

Created attachment 363047 [details]
Patch

[Dean Jackson]

Comment on attachment 363047 [details]
Patch

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

> Source/WebCore/Modules/webgpu/WebGPUBuffer.cpp:90
> +        arrayBuffer ? promise.resolve(*arrayBuffer) : promise.reject();

I suggest a real if statement here. I prefer ternaries only when assigning values.

> Source/WebCore/platform/graphics/gpu/GPUBuffer.h:120
> +    static const auto s_readOnlyMask = GPUBufferUsage::Index | GPUBufferUsage::Vertex | GPUBufferUsage::Uniform | GPUBufferUsage::TransferSrc;

Why do we need this as a static in the header now?

> Source/WebCore/platform/graphics/gpu/cocoa/GPUBufferMetal.mm:66
> +    // FIXME: Metal best practices: Read-only one-time-use data less than 4 KB should not allocate a MTLBuffer and be used in [MTLCommandEncoder set*Bytes] calls instead.

Nice.

> Source/WebCore/platform/graphics/gpu/cocoa/GPUBufferMetal.mm:118
> +    if (offset + data.byteLength() > m_byteLength) {

We need to use checked arithmetic here. It can be in a followup patch, but please add a FIXME.

Look for Checked<> stuff in WebGL or GraphicsContext3D as examples.

[Justin Fan]

*** Bug 194255 has been marked as a duplicate of this bug. ***

[Justin Fan]

Created attachment 363114 [details]
Patch for landing

[Justin Fan]

Created attachment 363115 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 363115 [details]
Patch for landing

Clearing flags on attachment: 363115

Committed r242148: <https://trac.webkit.org/changeset/242148>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Don Olmstead]

This patch broke Mac 32-bit builds. Just noticed because EWS is really backed up by this.

[Justin Fan]

(In reply to Don Olmstead from comment #9)
> This patch broke Mac 32-bit builds. Just noticed because EWS is really
> backed up by this.

You’re right. I landed some patches last night to try and turn off Web GPU on 32-bit (the problem is 64 bit to 32 bit narrowing conversion, and Metal is 64-bit only anyway) but I don’t think I’ve succeeded. Please stand by.

[Justin Fan]

(In reply to Don Olmstead from comment #9)
> This patch broke Mac 32-bit builds. Just noticed because EWS is really
> backed up by this.

Will be addressed in https://bugs.webkit.org/show_bug.cgi?id=195191.