Bug 199655 – [WebGPU] Implement GPUError and error scopes

• Component: WebGPU
• Status: RESOLVED FIXED

[Justin Fan]

[WebGPU] Implement validation errors and error scopes

[Justin Fan]

Created attachment 373817 [details]
Patch

[Justin Fan]

Created attachment 373819 [details]
Patch

[Justin Fan]

Created attachment 373820 [details]
Patch

[Myles C. Maxfield]

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?

[Justin Fan]

> > 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.

[Justin Fan]

Created attachment 373892 [details]
Patch

[Justin Fan]

Created attachment 373893 [details]
Patch

[Justin Fan]

Created attachment 373897 [details]
Patch

[Myles C. Maxfield]

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.

[Justin Fan]

(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.

[Justin Fan]

Created attachment 373933 [details]
Patch for landing

[Justin Fan]

Created attachment 373942 [details]
Patch for landing

[Justin Fan]

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.

[Justin Fan]

Comment on attachment 373942 [details]
Patch for landing

Clearing flags on attachment: 373942

Committed r247366: <https://trac.webkit.org/changeset/247366>

[Justin Fan]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/52972354>

[Ryan Haddad]

Rolled out in https://trac.webkit.org/r247442 because it broke the watchOS build.

[Justin Fan]

Re-landed in https://trac.webkit.org/changeset/247500/webkit.