Summary: | [WebGPU] Implement GPUError and error scopes | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Justin Fan <justin_fan> | ||||||||||||||||||
Component: | WebGPU | Assignee: | Justin Fan <justin_fan> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | commit-queue, mmaxfield, ryanhaddad, webkit-bug-importer | ||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||
Attachments: |
|
Description
Justin Fan
2019-07-09 20:43:14 PDT
Created attachment 373817 [details]
Patch
Created attachment 373819 [details]
Patch
Created attachment 373820 [details]
Patch
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? > > 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. Created attachment 373892 [details]
Patch
Created attachment 373893 [details]
Patch
Created attachment 373897 [details]
Patch
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. (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. Created attachment 373933 [details]
Patch for landing
Created attachment 373942 [details]
Patch for landing
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 on attachment 373942 [details] Patch for landing Clearing flags on attachment: 373942 Committed r247366: <https://trac.webkit.org/changeset/247366> All reviewed patches have been landed. Closing bug. Rolled out in https://trac.webkit.org/r247442 because it broke the watchOS build. Re-landed in https://trac.webkit.org/changeset/247500/webkit. |