Bug 191383 - [WebGPU] Code quality concerns raised for 191291: [WebGPU] Experimental prototype for WebGPURenderPipeline and WebGPUSwapChain
Summary: [WebGPU] Code quality concerns raised for 191291: [WebGPU] Experimental proto...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGPU (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Enhancement
Assignee: Justin Fan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-11-07 10:19 PST by Justin Fan
Modified: 2018-11-14 16:35 PST (History)
5 users (show)

See Also:


Attachments
Patch (21.01 KB, patch)
2018-11-14 11:20 PST, Justin Fan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-sierra-wk2 (3.32 MB, application/zip)
2018-11-14 16:07 PST, EWS Watchlist
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Justin Fan 2018-11-07 10:19:24 PST
Address code quality concerns for WebGPU after hello-triangle prototype is ready.

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

> Source/WebCore/Modules/webgpu/GPUPipelineDescriptorBase.h:37
> +    Vector<GPUPipelineStageDescriptor> stages;


Are you sure this is the right internal design? We need to differentiate between the vertex shader and the fragment shader when we compile the state. That shouldn't have to iterate through a list of things just to find which item is for the fragment shader.

> Source/WebCore/Modules/webgpu/GPUPipelineStageDescriptor.h:37
> +    const GPUShaderModule& module;


What happens when the module gets garbage collected? Will this be dangling?

> Source/WebCore/Modules/webgpu/GPURenderPipelineDescriptor.h:37
> +struct GPURenderPipelineDescriptor : GPUPipelineDescriptorBase {


Why struct?

> Source/WebCore/Modules/webgpu/GPURenderPipelineDescriptor.h:52
> +    int primitiveTopology;


This shouldn't be an int.

> Source/WebCore/Modules/webgpu/WebGPUPipelineStageDescriptor.h:39
> +    const WebGPUShaderModule* module = nullptr;


Ditto about the garbage collector

> Source/WebCore/Modules/webgpu/WebGPUShaderStage.h:36
> +    enum {


enum class

> Source/WebCore/Modules/webgpu/WebGPUShaderStage.h:39
> +        VERTEX = 0,
> +        FRAGMENT = 1,
> +        COMPUTE = 2


Can we use different internal names for these? Our style is not to use ALL CAPS.

> Source/WebCore/Modules/webgpu/cocoa/GPURenderPipeline.h:50
> +#else
> +using PlatformRenderPipeline = void;
> +using PlatformRenderPipelineSmartPtr = RefPtr<void>;
> +#endif


I think it's fine to assume we're not using metal for now. We can break that assumption one day when the linux ports want WebGPU.

> Source/WebCore/Modules/webgpu/cocoa/GPURenderPipelineMetal.mm:88
> +    BEGIN_BLOCK_OBJC_EXCEPTIONS;


We should be deliberate about where we use these.

> Source/WebCore/bindings/js/WebCoreBuiltinNames.h:182
> +    macro(WebGPURenderPipeline) \
> +    macro(WebGPUShaderStage) \


Why aren't the other objects in here?

> LayoutTests/webgpu/render-pipelines.html:5
> +<script id="library" type="x-shader/x-metal">


😐
Comment 1 Justin Fan 2018-11-07 23:16:21 PST
Note to self: A PipelineDescriptor retains ownership? of the shader modules since the original modules could very well fall out of scope before createRenderPipeline(pipelineDescriptor) is called, so fix the raw refs/ptrs.
Comment 2 Justin Fan 2018-11-14 11:01:33 PST
Note to past self: The JS ordered dictionaries created in order to represent the PipelineDescriptor will maintain references to any assigned ShaderModules, so the C++ structs do not have to maintain strong references.
Comment 3 Justin Fan 2018-11-14 11:20:49 PST
Created attachment 354839 [details]
Patch
Comment 4 Dean Jackson 2018-11-14 11:51:56 PST
Comment on attachment 354839 [details]
Patch

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

> Source/WebCore/Modules/webgpu/WebGPUDevice.cpp:70
> +    const char* const functionName = "WebGPUDevice::createRenderPipeline()";
> +#if LOG_DISABLED
> +    UNUSED_PARAM(functionName);
> +#endif

I think it's ok to duplicate the function name in each LOG statement.

> Source/WebCore/Modules/webgpu/WebGPUDevice.cpp:82
> +            LOG(WebGPU, "%s: Invalid WebGPUPipelineStageDescriptor!", functionName);

Do we need the !!!!s ? :)
Comment 5 Justin Fan 2018-11-14 12:11:50 PST
I like the !s in my errors messages! Are they bad?

Well, pulling out the function name made it a little easier to type at least, especially when I was refactoring stuff to cover error cases.
Comment 6 EWS Watchlist 2018-11-14 16:07:03 PST
Comment on attachment 354839 [details]
Patch

Attachment 354839 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9995083

New failing tests:
http/wpt/beacon/cors/cors-redirect-failure.html
Comment 7 EWS Watchlist 2018-11-14 16:07:05 PST
Created attachment 354866 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 8 WebKit Commit Bot 2018-11-14 16:34:03 PST
Comment on attachment 354839 [details]
Patch

Clearing flags on attachment: 354839

Committed r238208: <https://trac.webkit.org/changeset/238208>
Comment 9 WebKit Commit Bot 2018-11-14 16:34:04 PST
All reviewed patches have been landed.  Closing bug.
Comment 10 Radar WebKit Bug Importer 2018-11-14 16:35:34 PST
<rdar://problem/46080587>