Bug 199655 - [WebGPU] Implement GPUError and error scopes
Summary: [WebGPU] Implement GPUError and error scopes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGPU (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Justin Fan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-07-09 20:43 PDT by Justin Fan
Modified: 2019-07-16 17:04 PDT (History)
4 users (show)

See Also:


Attachments
Patch (51.49 KB, patch)
2019-07-09 21:30 PDT, Justin Fan
no flags Details | Formatted Diff | Diff
Patch (51.54 KB, patch)
2019-07-09 22:04 PDT, Justin Fan
no flags Details | Formatted Diff | Diff
Patch (51.58 KB, patch)
2019-07-09 22:15 PDT, Justin Fan
no flags Details | Formatted Diff | Diff
Patch (61.40 KB, patch)
2019-07-10 19:07 PDT, Justin Fan
no flags Details | Formatted Diff | Diff
Patch (61.38 KB, patch)
2019-07-10 19:09 PDT, Justin Fan
no flags Details | Formatted Diff | Diff
Patch (61.39 KB, patch)
2019-07-10 19:21 PDT, Justin Fan
no flags Details | Formatted Diff | Diff
Patch for landing (61.43 KB, patch)
2019-07-11 11:50 PDT, Justin Fan
no flags Details | Formatted Diff | Diff
Patch for landing (61.43 KB, patch)
2019-07-11 13:11 PDT, Justin Fan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Justin Fan 2019-07-09 20:43:14 PDT
[WebGPU] Implement validation errors and error scopes
Comment 1 Justin Fan 2019-07-09 21:30:32 PDT
Created attachment 373817 [details]
Patch
Comment 2 Justin Fan 2019-07-09 22:04:22 PDT
Created attachment 373819 [details]
Patch
Comment 3 Justin Fan 2019-07-09 22:15:50 PDT
Created attachment 373820 [details]
Patch
Comment 4 Myles C. Maxfield 2019-07-10 10:12:54 PDT
Comment on attachment 373820 [details]
Patch

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

> Source/WebCore/Modules/webgpu/WebGPUDevice.cpp:192
> +    m_device->popErrorScope([promise = WTFMove(promise)] (RefPtr<GPUError>&& error, bool isSuccess) mutable {

Why not Optional<RefPtr<GPUError>>&&?

> Source/WebCore/Modules/webgpu/WebGPUDevice.cpp:196
> +            promise.reject(Exception { OperationError, "GPUDevice::popErrorScope(): No error scope exists!"_s });

This message seems too specific. How do you know that the failure is due to a missing error scope? Shouldn't the GPUError have a string in it that gets placed here instead?

> Source/WebCore/platform/graphics/gpu/GPUBuffer.h:111
> +    static bool validateBufferUsage(GPUDevice&, OptionSet<GPUBufferUsage::Flags>);

Is it worth marking the error members of Device as mutable so you don't have to make this change?

> Source/WebCore/platform/graphics/gpu/GPUDevice.cpp:133
> +    // FIXME: Uncaptured errors need to be fired as GPUUncapturedErrorEvents.

Can we file a bug about this and link it here?

> Source/WebCore/platform/graphics/gpu/GPUError.cpp:42
> +        ASSERT_NOT_REACHED();

Can we just fill out the rest of this? We're going to be creating a lot of these things in the future and it would be convenient to not have to fill out this function then.

> Source/WebCore/platform/graphics/gpu/GPUError.h:40
> +using GPUError = GPUValidationError;

What? Why are all errors GPUValidationErrors? That seems not right. Why not make it a Variant or a class hierarchy with a virtual base class?
Comment 5 Justin Fan 2019-07-10 11:27:54 PDT
> > Source/WebCore/Modules/webgpu/WebGPUDevice.cpp:192
> > +    m_device->popErrorScope([promise = WTFMove(promise)] (RefPtr<GPUError>&& error, bool isSuccess) mutable {
> 
> Why not Optional<RefPtr<GPUError>>&&?
> 
> > Source/WebCore/Modules/webgpu/WebGPUDevice.cpp:196
> > +            promise.reject(Exception { OperationError, "GPUDevice::popErrorScope(): No error scope exists!"_s });
> 
> This message seems too specific. How do you know that the failure is due to
> a missing error scope? Shouldn't the GPUError have a string in it that gets
> placed here instead?

The design doc specs that this doesn't create an actual GPUError, but I can use the GPUError parameter of the callback to pass a rejection message back instead. I'm going to keep the 'isSuccess' parameter then (since if I used an optional I wouldn't be able to use the 'message' filed of GPUValidationError).

> > Source/WebCore/platform/graphics/gpu/GPUBuffer.h:111
> > +    static bool validateBufferUsage(GPUDevice&, OptionSet<GPUBufferUsage::Flags>);
> 
> Is it worth marking the error members of Device as mutable so you don't have
> to make this change?

Possibly.

> > Source/WebCore/platform/graphics/gpu/GPUDevice.cpp:133
> > +    // FIXME: Uncaptured errors need to be fired as GPUUncapturedErrorEvents.
> 
> Can we file a bug about this and link it here?

Will do. https://bugs.webkit.org/show_bug.cgi?id=199676

> > Source/WebCore/platform/graphics/gpu/GPUError.cpp:42
> > +        ASSERT_NOT_REACHED();
> 
> Can we just fill out the rest of this? We're going to be creating a lot of
> these things in the future and it would be convenient to not have to fill
> out this function then.
> 
> > Source/WebCore/platform/graphics/gpu/GPUError.h:40
> > +using GPUError = GPUValidationError;
> 
> What? Why are all errors GPUValidationErrors? That seems not right. Why not
> make it a Variant or a class hierarchy with a virtual base class?

Yes, that line is supposed to be 'using GPUError = Variant<GPUOutOfMemoryError, GPUValidationError>'. I'll add OOM in for this patch then.
Comment 6 Justin Fan 2019-07-10 19:07:25 PDT
Created attachment 373892 [details]
Patch
Comment 7 Justin Fan 2019-07-10 19:09:01 PDT
Created attachment 373893 [details]
Patch
Comment 8 Justin Fan 2019-07-10 19:21:35 PDT
Created attachment 373897 [details]
Patch
Comment 9 Myles C. Maxfield 2019-07-11 09:31:37 PDT
Comment on attachment 373897 [details]
Patch

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

> Source/WebCore/Modules/webgpu/WebGPUDevice.h:74
> +using ErrorPromise = DOMPromiseDeferred<IDLNullable<IDLUnion<IDLInterface<GPUOutOfMemoryError>, IDLInterface<GPUValidationError>>>>;

😱

Can we break this up into multiple lines of typedefs?

> Source/WebCore/platform/graphics/gpu/GPUError.cpp:39
> +        return GPUError(RefPtr<GPUOutOfMemoryError>(GPUOutOfMemoryError::create()));

I know I told you to do this, but you might want to consider making all errors the same type and using a member enum to differentiate them. Whatever you think is best.

> Source/WebCore/platform/graphics/gpu/GPUError.h:41
> +using GPUError = Variant<RefPtr<GPUOutOfMemoryError>, RefPtr<GPUValidationError>>;

Why not RefPtr<JustinsNewClass> which is ref counted and either hasa or isa variant? Or a single type with a member enum? It’s pretty awkward to put multiple RefPtrs in a variant.
Comment 10 Justin Fan 2019-07-11 11:36:17 PDT
(In reply to Myles C. Maxfield from comment #9)
> Comment on attachment 373897 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=373897&action=review
> 
> > Source/WebCore/Modules/webgpu/WebGPUDevice.h:74
> > +using ErrorPromise = DOMPromiseDeferred<IDLNullable<IDLUnion<IDLInterface<GPUOutOfMemoryError>, IDLInterface<GPUValidationError>>>>;
> 
> 😱
> 
> Can we break this up into multiple lines of typedefs?

Done.

> > Source/WebCore/platform/graphics/gpu/GPUError.cpp:39
> > +        return GPUError(RefPtr<GPUOutOfMemoryError>(GPUOutOfMemoryError::create()));
> 
> I know I told you to do this, but you might want to consider making all
> errors the same type and using a member enum to differentiate them. Whatever
> you think is best.

I'm going to keep this for now since it most closely matches the spec. The two error types do not share any fields right now.

> > Source/WebCore/platform/graphics/gpu/GPUError.h:41
> > +using GPUError = Variant<RefPtr<GPUOutOfMemoryError>, RefPtr<GPUValidationError>>;
> 
> Why not RefPtr<JustinsNewClass> which is ref counted and either hasa or isa
> variant? Or a single type with a member enum? It’s pretty awkward to put
> multiple RefPtrs in a variant.

Variant<RefPtr, RefPtr, ...> is how other WebCore code maps (RefCountedInterface or RefCountedInterface or ...). I think its possible to do this with raw pointers as well, but I don't know how that would work with object lifecycles.

Alternatives like Box<Variant<ErrorClass, ErrorClass2>> might work but could be a hassle to finagle into the Promise.resolve call.
Comment 11 Justin Fan 2019-07-11 11:50:46 PDT
Created attachment 373933 [details]
Patch for landing
Comment 12 Justin Fan 2019-07-11 13:11:36 PDT
Created attachment 373942 [details]
Patch for landing
Comment 13 Justin Fan 2019-07-11 14:13:24 PDT
commit queue is unable to pass tests without my patch, and ERS was all green this morning, so I'm going to land it manually.
Comment 14 Justin Fan 2019-07-11 14:32:09 PDT
Comment on attachment 373942 [details]
Patch for landing

Clearing flags on attachment: 373942

Committed r247366: <https://trac.webkit.org/changeset/247366>
Comment 15 Justin Fan 2019-07-11 14:32:11 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Radar WebKit Bug Importer 2019-07-11 14:33:23 PDT
<rdar://problem/52972354>
Comment 17 Ryan Haddad 2019-07-15 13:37:14 PDT
Rolled out in https://trac.webkit.org/r247442 because it broke the watchOS build.
Comment 18 Justin Fan 2019-07-16 17:04:54 PDT
Re-landed in https://trac.webkit.org/changeset/247500/webkit.