WebGPU: implement ComputeCommandEncoder and related components
Created attachment 306167 [details] Patch
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.
Created attachment 306455 [details] Patch
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.
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.
Created attachment 306477 [details] Patch
I fixed licenses and the test.
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/
(In reply to Myles C. Maxfield from comment #8) > This is the approach that our current implication has taken. s/implication/implementation/
Comment on attachment 306477 [details] Patch Given that disclaimer, let's do this!
Comment on attachment 306477 [details] Patch Clearing flags on attachment: 306477 Committed r215131: <http://trac.webkit.org/changeset/215131>
All reviewed patches have been landed. Closing bug.