Bug 201675 – Web Inspector: Canvas: show WebGPU shader pipelines

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

Since WebGPU has a similar vertex/fragment shader concept, the UI would likely be the same as for WebGL/WebGL2 (e.g. split view vertex and fragment).

Allowing live editing and recompilation would also be nice :)

[Devin Rousso]

Created attachment 378639 [details]
[Patch] WIP

This patch makes WebGPU shader pipelines (both render and compute) visible for the corresponding `WebGPUDevice`.  I still need to get editing working (including modifying the `entryPoint`, if possible), but this is already pretty great 😁

For compute shader pipelines, we don't need a split view as there's only one shader, so we only show a single content view with "Compute Shader" in the header.

This also needs tests -.-

[EWS Watchlist]

This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.

[Devin Rousso]

Created attachment 378640 [details]
[Image] After Patch is applied

[Devin Rousso]

Comment on attachment 378639 [details]
[Patch] WIP

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

> Source/WebInspectorUI/UserInterface/Views/ShaderProgramContentView.js:65
>                  textEditor.mimeType = "x-shader/x-vertex";

We should probably create another MIME type for compute shaders (e.g. "x-shader/x-compute"), but that isn't supported by CodeMirror right now (not to mention that "x-shader/x-vertex" and "x-shader/x-fragment" don't have proper syntax highlighting for WHLSL).

[Devin Rousso]

Created attachment 378718 [details]
[Patch] WIP

Reworked some things and added further scaffolding for editing.  The last bit remaining is to actually hook up editing (`GPUComputePipeline::recompile` and `GPURenderPipeline::recompile`).

[Devin Rousso]

Created attachment 378836 [details]
[Patch] WIP

[Devin Rousso]

Created attachment 378840 [details]
[Patch] WIP

[Devin Rousso]

Created attachment 378862 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 378862 [details]
Patch

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

r=me

Nice! Great tests.

The WebGPU Pipeline stuff went over my head so you might want to get a nod from a WebGPU expert.

WK1 tests seem to have failed, maybe WebGPU is not enabled there?:
CONSOLE MESSAGE: line 83: Unhandled Promise Rejection: TypeError: undefined is not an object (evaluating 'navigator.gpu.requestAdapter')

> Source/WebCore/inspector/InspectorShaderProgram.cpp:120
>          return nullptr;

Seems we should also ASSERT_NOT_REACHED for WebGL here.

> Source/WebCore/inspector/InspectorShaderProgram.cpp:231
> +                                if (entryPoint)
> +                                    shader.value().entryPoint = *entryPoint;

Is there any concern here about a malicious kind of entryPoint string?

> Source/WebCore/platform/graphics/gpu/GPUComputePipeline.h:70
> +    // Prserved for Web Inspector recompilation.

Typo: "Prserved" => "Preserved", This happens a few times.

> Source/WebInspectorUI/UserInterface/Models/ShaderProgram.js:45
> +    static _contextTypeSupportsProgramType(contextType, programType)

These are static, might as well not give them an underscore prefix.

> Source/WebInspectorUI/UserInterface/Models/ShaderProgram.js:118
> +        if (this._canvas.contextType === WI.Canvas.ContextType.WebGPU)
> +            return;
> +
> +        console.assert(this._programType === ShaderProgram.ProgramType.Render);
> +        console.assert(this._canvas.contextType === WI.Canvas.ContextType.WebGL || this._canvas.contextType === WI.Canvas.ContextType.WebGL2);

Maybe move the assertions above the early return?

That would help catch if we ever did this for a WebGPU context among other issues.

> Source/WebInspectorUI/UserInterface/Models/ShaderProgram.js:152
> +        console.assert((!!entryPoint) === (this._canvas.contextType === WI.Canvas.ContextType.WebGPU));

Style: Could drop the parenthesis on the left side.

> Source/WebInspectorUI/UserInterface/Views/ShaderProgramContentView.js:63
> +                textEditor.mimeType = "x-shader/x-vertex"; // Treat this like a vertex shader for CodeMirror.

You could probably add something to CodeMirrorAdditions.js:

    // FIXME: Add a WHLSL specific mode.
    // Treat a computer shader like a vertex shader.
    CodeMirror.defineMIME("x-shader/x-compute", "x-shader/x-vertex");

And keep this code rather clean.

> Source/WebInspectorUI/UserInterface/Views/ShaderProgramContentView.js:195
> +        case WI.Canvas.ContextType.WebGPU:
> +            extension = WI.unlocalizedString(".whlsl");

Is this the preferred extension? Perhaps `.wsl`? I was unable to find any concrete examples. whlsl seems find to me!

> Source/WebInspectorUI/UserInterface/Views/ShaderProgramTreeElement.js:55
> +            this.element.addEventListener("mouseover", this._handleMouseOver.bind(this));
> +            this.element.addEventListener("mouseout", this._handleMouseOut.bind(this));

I assume we will want to / should be able to do this for WebGPU in the future. So maybe add a FIXME.

> LayoutTests/ChangeLog:11
> +        * inspector/canvas/requestShaderSource-expected.txt:

It kind stinks when we have:

    inspector/foo/bar.html
    inspector/foo/bar-a.html
    inspector/foo/bar-b.html

That sorting gets ugly for:

    inspector/foo/bar-a-expected.html
    inspector/foo/bar-b-expected.html
    inspector/foo/bar-expected.html

Maybe we should rename this common tests to something like:

    inspector/canvas/requestShaderSource-invalid.html
    inspector/canvas/requestShaderSource-common.html

> LayoutTests/ChangeLog:12
> +        * inspector/canvas/updateShader.html:

Ditto

> LayoutTests/inspector/canvas/requestShaderSource-webgl.html:63
> +                InspectorTest.log(source);

`InspectorTest.ShaderProgram.logSource` instead of just `InspectorTest.log`?

> LayoutTests/inspector/canvas/resources/shaderProgram-utilities-webgpu.js:113
> +        InspectorTest.log("`" + source + "`");

Maybe "```\n" + source + "```"? That is more Markdown like output for generic code.

> LayoutTests/inspector/canvas/shaderProgram-add-remove-webgpu-expected.txt:15
> +PASS: Added "compute ShaderProgram.

Nit: Unbalanced quote

> LayoutTests/inspector/canvas/shaderProgram-add-remove-webgpu.html:29
> +                InspectorTest.expectThat(program instanceof WI.ShaderProgram, `Added "${programType} ShaderProgram.`);

Style: Unbalanced quote around ${programType}.

> LayoutTests/inspector/canvas/updateShader-webgpu.html:96
> +    test({
> +        name: "Canvas.updateShader.WebGPU.Fragment.Invalid",
> +        programType: WI.ShaderProgram.ProgramType.Render,
> +        shaderType: WI.ShaderProgram.ShaderType.Fragment,
> +        source: "INVALID",
> +        entryPoint: "INVALID",
> +        shouldThrow: true,
> +    });

What happens if you attempt to update with only an invalid or missing entryPoint?

[Devin Rousso]

Comment on attachment 378862 [details]
Patch

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

>> Source/WebCore/inspector/InspectorShaderProgram.cpp:120
>>          return nullptr;
> 
> Seems we should also ASSERT_NOT_REACHED for WebGL here.

I'd rather not, given that this is basically ingesting straight from the frontend, and there's nothing preventing an arbitrary frontend from sending "compute" for a WebGL program.  It is technically reachable.

>> Source/WebInspectorUI/UserInterface/Models/ShaderProgram.js:152
>> +        console.assert((!!entryPoint) === (this._canvas.contextType === WI.Canvas.ContextType.WebGPU));
> 
> Style: Could drop the parenthesis on the left side.

I would find that harder to read 😅

>> LayoutTests/inspector/canvas/requestShaderSource-webgl.html:63
>> +                InspectorTest.log(source);
> 
> `InspectorTest.ShaderProgram.logSource` instead of just `InspectorTest.log`?

That's only in the `-webgpu.js`.  I was trying to avoid modifying the expected results of the existing tests.

>> LayoutTests/inspector/canvas/resources/shaderProgram-utilities-webgpu.js:113
>> +        InspectorTest.log("`" + source + "`");
> 
> Maybe "```\n" + source + "```"? That is more Markdown like output for generic code.

Lol yup that'd be better :P

>> LayoutTests/inspector/canvas/updateShader-webgpu.html:96
>> +    });
> 
> What happens if you attempt to update with only an invalid or missing entryPoint?

I'm going to remove the ability to edit the `entryPoint`, as it's not really a "good" process and is likely not really something that the developer is going to change often.  I'd rather have a "Shader" details sidebar that has information about the shader program/pipeline, especially since WebGPU shader pipelines have a LOT more configuration than WebGL shader programs.

[Myles C. Maxfield]

Comment on attachment 378862 [details]
Patch

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

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPrepare.h:-41
> -// Hide the contents of the ShaderModule. Clients of the compiler shouldn't care what's inside it.

The details of the ShaderModule need to be hidden from WebCore. We expect to replace this string with some other compiler-dependent stuff. Instead, a better solution would be to create a wrapper object that contains a ShaderModule and a string. In the future, the string in the ShaderModule will be gone, but the other string can persist. (Even if there are two strings, they will both point to the same String Impl so the won't cause memory bloat.)

> Source/WebCore/Modules/webgpu/WebGPUComputePipeline.h:59
> +    // Prserved for Web Inspector recompilation.

typo

[Myles C. Maxfield]

I'm still in the middle of reviewing this.

[Justin Fan]

Comment on attachment 378862 [details]
Patch

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

>> Source/WebCore/inspector/InspectorShaderProgram.cpp:231
>> +                                    shader.value().entryPoint = *entryPoint;
> 
> Is there any concern here about a malicious kind of entryPoint string?

You mean, shader code? This string here is just used to match the contents of the shader; any shader code that can run will first be vetted by the compiler.

[Devin Rousso]

Comment on attachment 378862 [details]
Patch

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

>> LayoutTests/ChangeLog:11
>> +        * inspector/canvas/requestShaderSource-expected.txt:
> 
> It kind stinks when we have:
> 
>     inspector/foo/bar.html
>     inspector/foo/bar-a.html
>     inspector/foo/bar-b.html
> 
> That sorting gets ugly for:
> 
>     inspector/foo/bar-a-expected.html
>     inspector/foo/bar-b-expected.html
>     inspector/foo/bar-expected.html
> 
> Maybe we should rename this common tests to something like:
> 
>     inspector/canvas/requestShaderSource-invalid.html
>     inspector/canvas/requestShaderSource-common.html

I agree that the sorting isn't ideal, but I don't like adding a `-common`/`-invalid` suffix.  I'd rather the common things be in the file that matches the protocol name so it's the most straightforward to find, and then any "specializations" have a `-*` suffix at the end.

[Devin Rousso]

Created attachment 378927 [details]
Patch

Reworked shader module source logic to not expose the `WHLSL::ShaderModule`.

[Dean Jackson]

Comment on attachment 378927 [details]
Patch

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

this looks LGTM to me

> Source/WebCore/Modules/webgpu/WebGPUComputePipeline.h:50
> +    Optional<WebGPUPipeline::ShaderData> computeShader() const { return m_computeShader; }

I wonder if this should just be shader()?

> Source/WebCore/Modules/webgpu/WebGPUComputePipeline.h:52
> +    bool recompile(const WebGPUDevice&);

Why not just compile(..)?

> Source/WebCore/Modules/webgpu/WebGPUComputePipeline.h:60
> +    Optional<WebGPUPipeline::ShaderData> m_computeShader;

m_shader;

> Source/WebCore/Modules/webgpu/WebGPUPipeline.cpp:48
> +Lock& WebGPUPipeline::instancesMutex()

WebGPUPipeline::mutex() ?

[Devin Rousso]

Comment on attachment 378927 [details]
Patch

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

>> Source/WebCore/Modules/webgpu/WebGPUComputePipeline.h:50
>> +    Optional<WebGPUPipeline::ShaderData> computeShader() const { return m_computeShader; }
> 
> I wonder if this should just be shader()?

I like keeping the `compute*` prefix, so it's clear that the shader is expected to be a compute shader.  Also, many other values are also prefixed with `compute*` (e.g. `m_computePipeline`).

>> Source/WebCore/Modules/webgpu/WebGPUComputePipeline.h:52
>> +    bool recompile(const WebGPUDevice&);
> 
> Why not just compile(..)?

I wanted some distinction/indication that this doesn't need to be called after construction, and is instead a way of basically "updating" the pipeline.  Perhaps `update` would be better?

>> Source/WebCore/Modules/webgpu/WebGPUPipeline.cpp:48
>> +Lock& WebGPUPipeline::instancesMutex()
> 
> WebGPUPipeline::mutex() ?

This matches `WebGLProgram::instancesMutex` and `CanvasRenderingContext::instancesMutex` and `WebGPUDevice::instancesMutex`.

[Devin Rousso]

Comment on attachment 378927 [details]
Patch

I spoke with Myles offline, and he said to go ahead and land this.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 378927 [details]:

media/modern-media-controls/compact-media-controls/compact-media-controls-constructor.html bug 193587 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 378927 [details]
Patch

Clearing flags on attachment: 378927

Committed r250114: <https://trac.webkit.org/changeset/250114>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/55543450>

[Truitt Savell]

It looks like the changes in https://trac.webkit.org/changeset/250114/webkit

has broken ~16 tests on Mojave Release wk2.

Build:
https://build.webkit.org/builders/Apple%20Mojave%20Release%20WK2%20%28Tests%29/builds/6794

I was able to reproduce these crashes locally by just running the webgpu/ directory on 250113 which passed and 250114 which failed. 

Results:
https://build.webkit.org/results/Apple%20Mojave%20Release%20WK2%20(Tests)/r250114%20(6794)/results.html

[Alexey Proskuryakov]

This also appears to have caused flaky crashes on WebKit1, see bug 202038 and bug 202046.

Seems like we should do an emergency rollback due to impact on EWS and commit queue.

[Truitt Savell]

Reverted r250114 for reason:

Broke ~16 webgpu/ tests on Mojave wk2

Committed r250139: <https://trac.webkit.org/changeset/250139>

[Devin Rousso]

Created attachment 379387 [details]
Patch

Remove some invalid `ASSERT` (the `layout` of a pipeline descriptor can be left out).

[Devin Rousso]

Comment on attachment 379387 [details]
Patch

Oops, this still has some debug logging :(

[Devin Rousso]

Created attachment 379393 [details]
Patch

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 379393 [details]:

inspector/canvas/resolveContext-bitmaprenderer.html bug 202038 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 379393 [details]
Patch

Clearing flags on attachment: 379393

Committed r250258: <https://trac.webkit.org/changeset/250258>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

This is causing crashes again, see bug 202186.