| Summary: | Stop building WebGPU and the WHLSL compiler to decrease binary size | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Myles C. Maxfield <mmaxfield> | ||||||||||||||||||||
| Component: | New Bugs | Assignee: | Myles C. Maxfield <mmaxfield> | ||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||
| Severity: | Normal | CC: | annulen, bburg, benjamin, calvaris, cdumez, changseok, cmarcelo, darin, dino, don.olmstead, eric.carlson, esprehn+autocc, ews-watchlist, fred.wang, glenn, gyuyoung.kim, hi, hta, jer.noble, joepeck, kangil.han, keith_miller, kondapallykalyan, mark.lam, msaboff, pdr, philipj, ryuan.choi, saam, sam, sergio, simon.fraser, tommyw, tzagallo, webkit-bug-importer, webkit-bugs.rumbling202, youennf | ||||||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=228623 | ||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||
|
Description
Myles C. Maxfield
2021-07-22 02:08:05 PDT
Created attachment 433998 [details]
Patch
Having code that isn't compiled isn't usually something we like to do. Perhaps we should instead remove it. We need a solution for GPU_DRIVER_PREWARMING. Created attachment 434414 [details]
Patch
This patch modifies the inspector protocol generator. Please ensure that you have rebaselined any generator test results (i.e., by running `Tools/Scripts/run-inspector-generator-tests --reset-results`) This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features. Created attachment 434415 [details]
Patch
Created attachment 434483 [details]
Patch
Created attachment 434506 [details]
Patch
Created attachment 434514 [details]
Patch
Comment on attachment 434514 [details]
Patch
GTK failure looks unrelated
Created attachment 434565 [details]
Patch
The Dawn stuff is currently just stubs so get rid of those as well. The EWS won’t catch any build errors since it’s not enabled by default. Also you’ll want to grep the CMake stuff for anything ENABLE_WEBGPU. I think you can probably ignore the Source/cmake directory in that since eventually this will all be rebooted and that code would need to go back. Created attachment 434569 [details]
Patch
I presume the WHLSL is not coming back, but WebGPU? I am a little confused about the status of that. Our current implementation of WebGPU: 1. Is off by default on all platforms 2. Is extremely outdated 3. Has no notion of the GPU Process, and therefore needs to be redesigned and largely rewritten 4. Only implements a fraction of what is in the spec Removing the code from the tree doesn't delete it from existence; it's still in source control. The benefit of reducing binary size seems to outweigh having this code in the tree. Sure, Myles, I wasn’t questioning the technique. I was wondering what our WebGPU development strategy was for WebKit. Thanks for filling me in. I need to re-revert the changes to Source/WebInspectorUI LGTM, but I'd rather let a Webcore developer give this the r+. I think our intentions may be a bit muddled here; let me try to make them clear. 1. We desire to support WebGPU in WebKit. If somehow an oracle delivered a high-quality implementation of WebGPU today, we would definitely incorporate it and enable it! 2. We do not desire to support WHLSL in WebKit. WGSL is the path forward. 3. I don't believe there should/will be any development-practices kind of change to the development of WebGPU in WebKit. Development doesn't need to be done on a branch - our usual process of landing patches one by one to trunk continues to make sense. 4. Any implementation of WebGPU in WebKit would have to be GPU-Process-savvy. 5. It is still a good idea for the implementation of WebGPU to be split into two parts: a frontend part that knows things about IDL and promises and javascript, and a backend part that knows things about the backend APIs (like Metal or D3D or Vulkan). The reason is that we may want to implement future features in WebKit (maybe GPU-accelerated SVG filters???) on top of the backend part, without involving Javascript at all. The current implementation puts the frontend in Modules/WebGPU and the backend in platform/graphics/gpu, but those paths aren't set in stone. 6. This patch deletes the existing implementation just to reduce binary size in releases of WebKit which don't enable WebGPU by default. It doesn't represent a change in direction or policy or anything regarding WebGPU. It's a (somewhat temporary) pragmatic change. (In reply to Myles C. Maxfield from comment #19) > I need to re-revert the changes to Source/WebInspectorUI Yeah, unless your goal is to remove Web Inspector support for WebGPU/WHLSL when inspecting iOS 14.0/14.5/15.0 devices, please undo all your changes inside `Source/WebInspectorUI`. FWIW it's not unprecedented to also remove Web Inspector backwards compatibility support for features when they're removed from ToT, but we usually prefer not to do that with the goal of keeping Web Inspector as much "as it was" (specifically the functionality, not the UI) as possible. Also, the JSON files you modified are not actually used anywhere in the frontend. They're purely for record keeping and archival purposes. If you did want to do the above, you'd actually want to modify the various `Source/WebInspectorUI/UserInterface/Protocol/Legacy/*/InspectorBackendCommands.js` files as that's actually used by the frontend. The JSON files are used to generate the JS files, but the JS file doesn't have everything in the JSON file so we keep the JSON file around so we can reference it when needed. I personally think we should keep supporting Web Inspector backwards compatibility, so I'm in favor of allowing the Web Inspector frontend to still see/instrument/etc. WebGPU/WHLSL when inspecting iOS 14.0/14.5/15.0, but that's just my thoughts. Also the inspector changes in WebCore look good to me :) Created attachment 434588 [details]
Patch
I added WebGPU to webkit.org/status in https://commits.webkit.org/r280456 with a status of "in progress". Comment on attachment 434588 [details]
Patch
Can you mention in the change log that this isn't because we don't want to support WebGPU, but rather that this implementation is incorrect and not serving any purpose (other than slowing down the build). We'll implement a better version.
Also, you say this is for Cocoa, but also remove the Dawn code that was added by other contributors. I guess this makes sense since you're removing the feature.
I'm probably going to have to land this manually, because the patch is so large Committed r280467 (240102@main): <https://commits.webkit.org/240102@main> I initially was trying to view the WebKit WebGPU demos here: https://webkit.org/demos/webgpu/ However, I get an error "WebGPU not available", and instructions on how to enable it in Safari. After looking around WebKit Bugzilla, I was linked to this bug from another: https://bugs.webkit.org/show_bug.cgi?id=243279 Regarding this quote: > Can you mention in the change log that this isn't because we don't want to support WebGPU, but rather that this implementation is incorrect and not serving any purpose (other than slowing down the build). We'll implement a better version. Which bug or external page is best to follow the progress of WebGPU in WebKit/Safari? I found a few open tickets, but am not sure which one to follow: https://bugs.webkit.org/buglist.cgi?bug_status=ASSIGNED&bug_status=REOPENED&bug_status=VERIFIED&component=WebGPU&list_id=9549204&product=WebKit&query_format=advanced |