Bug 201675 - Web Inspector: Canvas: show WebGPU shader pipelines
Summary: Web Inspector: Canvas: show WebGPU shader pipelines
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on: 201650
Blocks: 201855 201856 202030 202031
  Show dependency treegraph
 
Reported: 2019-09-10 20:25 PDT by Devin Rousso
Modified: 2019-09-25 15:21 PDT (History)
22 users (show)

See Also:


Attachments
[Patch] WIP (104.01 KB, patch)
2019-09-12 03:38 PDT, Devin Rousso
drousso: commit-queue-
Details | Formatted Diff | Diff
[Image] After Patch is applied (814.28 KB, image/png)
2019-09-12 03:39 PDT, Devin Rousso
no flags Details
[Patch] WIP (110.14 KB, patch)
2019-09-13 00:31 PDT, Devin Rousso
drousso: commit-queue-
Details | Formatted Diff | Diff
[Patch] WIP (136.49 KB, patch)
2019-09-15 21:33 PDT, Devin Rousso
drousso: commit-queue-
Details | Formatted Diff | Diff
[Patch] WIP (137.39 KB, patch)
2019-09-15 22:32 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (205.45 KB, patch)
2019-09-16 07:59 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (207.37 KB, patch)
2019-09-16 19:11 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (211.62 KB, patch)
2019-09-23 13:23 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (209.61 KB, patch)
2019-09-23 14:11 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2019-09-10 20:25:42 PDT
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 :)
Comment 1 Devin Rousso 2019-09-12 03:38:52 PDT
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 -.-
Comment 2 EWS Watchlist 2019-09-12 03:39:40 PDT
This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.
Comment 3 Devin Rousso 2019-09-12 03:39:50 PDT
Created attachment 378640 [details]
[Image] After Patch is applied
Comment 4 Devin Rousso 2019-09-12 03:41:34 PDT
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).
Comment 5 Devin Rousso 2019-09-13 00:31:51 PDT
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`).
Comment 6 Devin Rousso 2019-09-15 21:33:31 PDT
Created attachment 378836 [details]
[Patch] WIP
Comment 7 Devin Rousso 2019-09-15 22:32:04 PDT
Created attachment 378840 [details]
[Patch] WIP
Comment 8 Devin Rousso 2019-09-16 07:59:55 PDT
Created attachment 378862 [details]
Patch
Comment 9 Joseph Pecoraro 2019-09-16 15:47:11 PDT
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?
Comment 10 Devin Rousso 2019-09-16 17:09:28 PDT
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.
Comment 11 Myles C. Maxfield 2019-09-16 17:42:33 PDT
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
Comment 12 Myles C. Maxfield 2019-09-16 17:42:53 PDT
I'm still in the middle of reviewing this.
Comment 13 Justin Fan 2019-09-16 18:06:18 PDT
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.
Comment 14 Devin Rousso 2019-09-16 18:30:22 PDT
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.
Comment 15 Devin Rousso 2019-09-16 19:11:39 PDT
Created attachment 378927 [details]
Patch

Reworked shader module source logic to not expose the `WHLSL::ShaderModule`.
Comment 16 Dean Jackson 2019-09-17 01:50:11 PDT
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() ?
Comment 17 Devin Rousso 2019-09-17 07:30:21 PDT
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`.
Comment 18 Devin Rousso 2019-09-19 17:39:12 PDT
Comment on attachment 378927 [details]
Patch

I spoke with Myles offline, and he said to go ahead and land this.
Comment 19 WebKit Commit Bot 2019-09-19 18:19:22 PDT
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: graouts@apple.com)
The commit-queue is continuing to process your patch.
Comment 20 WebKit Commit Bot 2019-09-19 18:20:39 PDT
Comment on attachment 378927 [details]
Patch

Clearing flags on attachment: 378927

Committed r250114: <https://trac.webkit.org/changeset/250114>
Comment 21 WebKit Commit Bot 2019-09-19 18:20:41 PDT
All reviewed patches have been landed.  Closing bug.
Comment 22 Radar WebKit Bug Importer 2019-09-19 18:21:25 PDT
<rdar://problem/55543450>
Comment 23 Truitt Savell 2019-09-20 08:40:09 PDT
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
Comment 24 Alexey Proskuryakov 2019-09-20 12:13:11 PDT
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.
Comment 25 Truitt Savell 2019-09-20 12:39:48 PDT
Reverted r250114 for reason:

Broke ~16 webgpu/ tests on Mojave wk2

Committed r250139: <https://trac.webkit.org/changeset/250139>
Comment 26 Devin Rousso 2019-09-23 13:23:48 PDT
Created attachment 379387 [details]
Patch

Remove some invalid `ASSERT` (the `layout` of a pipeline descriptor can be left out).
Comment 27 Devin Rousso 2019-09-23 13:57:28 PDT
Comment on attachment 379387 [details]
Patch

Oops, this still has some debug logging :(
Comment 28 Devin Rousso 2019-09-23 14:11:17 PDT
Created attachment 379393 [details]
Patch
Comment 29 WebKit Commit Bot 2019-09-23 15:26:03 PDT
The commit-queue encountered the following flaky tests while processing attachment 379393 [details]:

inspector/canvas/resolveContext-bitmaprenderer.html bug 202038 (author: drousso@apple.com)
The commit-queue is continuing to process your patch.
Comment 30 WebKit Commit Bot 2019-09-23 15:27:28 PDT
Comment on attachment 379393 [details]
Patch

Clearing flags on attachment: 379393

Committed r250258: <https://trac.webkit.org/changeset/250258>
Comment 31 WebKit Commit Bot 2019-09-23 15:27:31 PDT
All reviewed patches have been landed.  Closing bug.
Comment 32 Alexey Proskuryakov 2019-09-25 15:21:17 PDT
This is causing crashes again, see bug 202186.