Bug 195077 - [Web GPU] Buffer updates part 2: setSubData, GPU/CPU synchronization
Summary: [Web GPU] Buffer updates part 2: setSubData, GPU/CPU synchronization
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGPU (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Justin Fan
URL:
Keywords: InRadar
: 194255 (view as bug list)
Depends on:
Blocks:
 
Reported: 2019-02-26 16:43 PST by Justin Fan
Modified: 2019-02-28 16:23 PST (History)
9 users (show)

See Also:

Attachments
Patch (64.25 KB, patch)
2019-02-26 17:43 PST, Justin Fan 		no flags Details | Formatted Diff | Diff
Patch for landing (70.17 KB, patch)
2019-02-27 12:32 PST, Justin Fan 		no flags Details | Formatted Diff | Diff
Patch for landing (70.24 KB, patch)
2019-02-27 12:33 PST, Justin Fan 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Justin Fan 2019-02-26 16:43:28 PST 
[Web GPU] Buffer updates part 2: setSubData, GPU/CPU synchronization
Comment 1 Justin Fan 2019-02-26 16:43:46 PST 
<rdar://problem/47805229>
Comment 2 Justin Fan 2019-02-26 17:43:27 PST 
Created attachment 363047 [details]
Patch
Comment 3 Dean Jackson 2019-02-27 11:41:24 PST 
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.
Comment 4 Justin Fan 2019-02-27 11:51:54 PST 
*** Bug 194255 has been marked as a duplicate of this bug. ***
Comment 5 Justin Fan 2019-02-27 12:32:48 PST 
Created attachment 363114 [details]
Patch for landing
Comment 6 Justin Fan 2019-02-27 12:33:25 PST 
Created attachment 363115 [details]
Patch for landing
Comment 7 WebKit Commit Bot 2019-02-27 13:10:30 PST 
Comment on attachment 363115 [details]
Patch for landing

Clearing flags on attachment: 363115

Committed r242148: <https://trac.webkit.org/changeset/242148>
Comment 8 WebKit Commit Bot 2019-02-27 13:10:32 PST 
All reviewed patches have been landed.  Closing bug.
Comment 9 Don Olmstead 2019-02-28 09:28:28 PST 
This patch broke Mac 32-bit builds. Just noticed because EWS is really backed up by this.
Comment 10 Justin Fan 2019-02-28 11:23:27 PST 
(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.
Comment 11 Justin Fan 2019-02-28 16:23:11 PST 
(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.