Bug 170444 – WebGPU: implement ComputeCommandEncoder and related components

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Yuichiro Kikura]

WebGPU: implement ComputeCommandEncoder and related components

[Yuichiro Kikura]

Created attachment 306167 [details]
Patch

[Alex Christensen]

Comment on attachment 306167 [details]
Patch

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

This is really cool!
A few thoughts:
1) This needs to compile successfully on the iOS simulator for us to commit it.  See https://webkit.org/blog/3457/building-webkit-for-ios-simulator/ for help figuring out how.
2) I'm not sure if bounds checks or other validation steps will be needed.  Imagine a malicious web developer trying to overflow a buffer somewhere.
3) There should be a test for this.  I'm not sure what the status of the WebGPU tests are because it's in a prototyping state right now, but we will need to have tests that cover the new functionality to make sure it is accessible from javascript and has the correct behavior.  See LayoutTests/fast/canvas/webgpu which has some serious room for improvement.

> Source/WebCore/ChangeLog:8
> +        I implemented WebGPUComputeCommandEncoder and related components based on the WebGPU proposal.

Could you put a link to the proposal?  I haven't been following closely enough to know what you're talking about.

> Source/WebCore/ChangeLog:21
> +        If my understand is right, WebKit doesn't support promise attribute.

If we don't support it and it's needed, we might need to implement it.  Is something missing in the bindings generators?  See Source/WebCore/bindings/scripts/generate-bindings.pl and the other scripts there.

[Yuichiro Kikura]

Created attachment 306455 [details]
Patch

[Yuichiro Kikura]

Dear Alex Christensen,

Thank you for the comment. I fixed bugs and problems you pointed out. The problem about Promise attribute is just my misunderstanding, sorry. 

The problem about buffer-overrun may still remain, but sorry, I'm not sure. I just used WebGPUBuffer implementation which had been implemented already.

[Alex Christensen]

Comment on attachment 306455 [details]
Patch

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

I'd appreciate Dean looking at this.

> Source/WebCore/bindings/js/JSWebGPUCommandBufferCustom.cpp:2
> + * Copyright (C) 2016 Apple Inc. All rights reserved.

This is inconsistent.

> Source/WebCore/platform/graphics/cocoa/GPUComputeCommandEncoderMetal.mm:2
> + * Copyright (C) 2017 Apple Inc. All rights reserved.

Ditto

> LayoutTests/fast/canvas/webgpu/webgpu-dispatch.html:38
> +        window.testRunner.notifyDone();

This doesn't make sense here.

[Yuichiro Kikura]

Created attachment 306477 [details]
Patch

[Yuichiro Kikura]

I fixed licenses and the test.

[Myles C. Maxfield]

I think this is good and we probably want to accept this.

However, we should probably be clear about some things:

- Our implementation of WebGPU is just an experiment. It is designed to help us understand what kind of performance is possible. It is also designed to help us understand any design issues which may appear.

- The WebKit project tries to be standards-compliant where possible. We are currently involved in the "GPU on the Web"[1] standardization group. This group is still very young, and does not have a draft specification document. Therefore, as this group develops, our implementation will probably change dramatically. The changes will probably include both fundamental design as well as details of implementation.

- The apparent direction of this patch is to mirror the Metal API in Javascript. This is the approach that our current implication has taken. However, we will switch approaches to, instead, pursue the work being done in the standardization group. Therefore, this patch is acceptable because it is an extension of what we have already done, but the existing approach, including this patch, will likely have to be dramatically changed soon.

[1] https://www.w3.org/community/gpu/

[Myles C. Maxfield]

(In reply to Myles C. Maxfield from comment #8)
> This is the approach that our current implication has taken.

s/implication/implementation/

[Alex Christensen]

Comment on attachment 306477 [details]
Patch

Given that disclaimer, let's do this!

[WebKit Commit Bot]

Comment on attachment 306477 [details]
Patch

Clearing flags on attachment: 306477

Committed r215131: <http://trac.webkit.org/changeset/215131>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.