Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add AsWebGpuErrorType and ErrorType APIs #6547

Draft
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

ErichDonGubler
Copy link
Member

@ErichDonGubler ErichDonGubler commented Nov 14, 2024

Connections

Description
Describe what problem this is solving, and how it's solved.

TODO:

  • Make Deno consume this.
  • Check that Firefox can consume this.
  • Inform Servo when this merges.

Testing
Explain how this change is tested.

Checklist

  • Run cargo fmt.
  • Run taplo format.
  • Run cargo clippy. If applicable, add:
    • --target wasm32-unknown-unknown
    • --target wasm32-unknown-emscripten
  • Run cargo xtask test to run tests.
  • Add change to CHANGELOG.md. See simple instructions inside file.

@ErichDonGubler ErichDonGubler added the type: enhancement New feature or request label Nov 14, 2024
@ErichDonGubler ErichDonGubler self-assigned this Nov 14, 2024
@ErichDonGubler ErichDonGubler force-pushed the webgpu-error-type branch 3 times, most recently from 93cf61c to 97ee033 Compare November 14, 2024 22:32
@teoxoy

This comment was marked as resolved.

@sagudev
Copy link
Contributor

sagudev commented Nov 16, 2024

This will simplify error handling in servo too, currently we hack it: https://github.com/servo/servo/blob/ee63174d6ff0b3b7d9b255fc47c72a82ae63bc09/components/webgpu/gpu_error.rs#L76

That reminds me this should also be used for classification in wgpu:

fn handle_error_inner(
&self,
sink_mutex: &Mutex<ErrorSinkRaw>,
source: ContextErrorSource,
label: Label<'_>,
fn_ident: &'static str,
) {
let source_error: ErrorSource = Box::new(wgc::error::ContextError {
fn_ident,
source,
label: label.unwrap_or_default().to_string(),
});
let mut sink = sink_mutex.lock();
let mut source_opt: Option<&(dyn Error + 'static)> = Some(&*source_error);
let error = loop {
if let Some(source) = source_opt {
if let Some(wgc::device::DeviceError::OutOfMemory) =
source.downcast_ref::<wgc::device::DeviceError>()
{
break crate::Error::OutOfMemory {
source: source_error,
};
}
source_opt = source.source();
} else {
// Otherwise, it is a validation error
break crate::Error::Validation {
description: self.format_error(&*source_error),
source: source_error,
};
}
};
sink.handle_error(error);
}

@ErichDonGubler ErichDonGubler force-pushed the webgpu-error-type branch 2 times, most recently from 864fab4 to e5121e0 Compare November 18, 2024 21:40
#[repr(u8)]
#[derive(Clone, Copy, Debug)]
#[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))]
pub enum ErrorType {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought: This should probably go into wgpu_types? Not sure.

match self {
DeviceError::DeviceMismatch(e) => e.as_webgpu_error_type(),
Self::Invalid(_) => ErrorType::Validation,
Self::Lost | Self::ResourceCreationFailed => ErrorType::Internal,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought: I'm minorly concerned here about the Lost variant of this type. This seems to have meaningful handling in Firefox. Perhaps this is a variant we need to add to ErrorType?

Copy link
Contributor

@sagudev sagudev Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In servo we have this: https://github.com/servo/servo/blob/ba061ec2b0ef7124a5e64ec11a406cbc45cac02f/components/webgpu/wgpu_thread.rs#L389, I think instead of adding new variant, as_webgpu_error_type could return Option when wgpu error is not considered as error.

EDIT: Hm, this sounds like ErrorType::Internal.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sagudev: Internal errors are in the same error hierarchy as OOM and validation errors. Device loss, however, is usually reported through. I need to investigate whether reporting device loss as something distinct from an internal error is still necessary, if that's properly hooked up.

CC @teoxoy and @jimblandy for Firefox thinking.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we now handle driver induced device loss (#6229) properly we shouldn't need to do anything special with the lost variant anymore. It can be ignored.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still need an error type for it I guess but that can also be ignored in downstream code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement New feature or request
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

3 participants