deno: Fix error reporting for async pipeline creation #8712
Conversation
jimblandy
left a comment
jimblandy
left a comment
There was a problem hiding this comment.
I'm not the deno_webgpu owner, but these changes look good to me.
| #[derive(Clone, Copy)] | ||
| pub(crate) struct GPUTextureUsageFlags(pub(crate) wgpu_types::TextureUsages); | ||
|
|
||
| impl<'a> WebIdlConverter<'a> for GPUTextureUsageFlags { |
There was a problem hiding this comment.
Certainly doing it this way is better than performing ad-hoc fallible conversions elsewhere. But these three converters are pretty repetitive. If there are other flag-like types that would benefit from this treatment, it might be worth figuring out some way to do this with generic code, or at least a macro.
There was a problem hiding this comment.
I agree, my only hesitation is that there might even be something out-of-the-box that can do it if we investigate. But otherwise a macro is a good idea.
andyleiserson
force-pushed
the
push-mxnpupsnorln
branch
from
d8ddbd0 to
5c40b8a
Compare
| pub(crate) fn make_pipeline_error<'a>( | ||
| scope: &mut v8::HandleScope<'a>, | ||
| reason: GPUPipelineErrorReason, | ||
| message: &str, | ||
| ) -> v8::Local<'a, v8::Object> { | ||
| let state = JsRuntime::op_state_from(scope); | ||
| let class = state | ||
| .borrow() | ||
| .borrow::<crate::PipelineErrorClass>() | ||
| .0 | ||
| .clone(); | ||
| let constructor = | ||
| v8::Local::<v8::Function>::try_from(v8::Local::new(scope, class)).unwrap(); | ||
| let message_str = v8::String::new(scope, message).unwrap(); | ||
| let reason_str = v8::String::new(scope, &reason.to_string()).unwrap(); | ||
|
|
||
| let options = v8::Object::new(scope); | ||
| let key = v8::String::new(scope, "reason").unwrap(); | ||
| options.set(scope, key.into(), reason_str.into()); | ||
|
|
||
| constructor | ||
| .new_instance(scope, &[message_str.into(), options.into()]) | ||
| .unwrap() | ||
| } |
There was a problem hiding this comment.
thought: It's so annoying that we have to work around the error registry system this way. We're definitely not the first to run into this pain…but I'm not sure what solutions upstream would accept to even start to tackle it. 😐
There was a problem hiding this comment.
I actually didn't see a lot of precedent for this so I'd be interested what you were looking at. Most of the error handling stuff seemed to be about ways to wrap Rust errors for JS, which didn't seem like what we want here since we want to produce a precise spec-defined thing.
4 participants
Of
createRenderPipelineAsync, the spec says:This changes the deno bindings to do as it says.
Ancillary note: Since pipeline creation is not actually async, the only reason to use these functions would be to follow the spec's recommendation that they are preferred, possibly in hopes of a future performance improvement.
To simplify the pipeline creation functions, this change also adds some WebIDL converters, which eliminates an explicit error return from
new_render_pipeline. These changes are in separate commits.Testing
By the CTS.
Squash or Rebase? Rebase (after squashing any fixups)
Checklist
cargo fmt.taplo format.cargo clippy --tests. If applicable, add:--target wasm32-unknown-unknowncargo xtask testto run tests.