Skip to content

Convert as_hal calls to use guards intead of callbacks #7863

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

Merged
merged 4 commits into from
Jul 9, 2025

Conversation

cwfitzgerald
Copy link
Member

Description

This converts all as_hal calls (except CommandBuffers) from callbacks to guards. This will make the api signifigantly better.

I needed to add a few escape hatches in the locking infrastructure to allow dependent locks to exist in a single guard. All of the unsafe in this PR does not affect Mozilla.

Review experience is commit by commit

Testing

No current testing of the as_hal api at all. Working on it.

Squash or Rebase?

Rebase

Checklist

  • Run cargo fmt.
  • Run taplo format.
  • Run cargo clippy --tests. If applicable, add:
    • --target wasm32-unknown-unknown
  • Run cargo xtask test to run tests.
  • If this contains user-facing changes, add a CHANGELOG.md entry.

@cwfitzgerald cwfitzgerald requested a review from a team as a code owner June 29, 2025 05:23
@cwfitzgerald cwfitzgerald changed the title [core] Move as_hal methods to their own file Convert as_hal calls to use guards intead of callbacks Jun 29, 2025
@cwfitzgerald cwfitzgerald force-pushed the cw/as_hal_guards branch 2 times, most recently from e57540a to f58ac37 Compare June 29, 2025 05:28
@Vecvec Vecvec requested a review from a team June 29, 2025 05:47
@Vecvec
Copy link
Collaborator

Vecvec commented Jun 29, 2025

I'm not the one doing the review, I just noticed a typo.

@JMS55
Copy link
Collaborator

JMS55 commented Jun 29, 2025

Do you still have to unwrap() manually in case of backend API mismatch?

@cwfitzgerald
Copy link
Member Author

Do you still have to unwrap() manually in case of backend API mismatch?

Yes all apis return options.

@JMS55
Copy link
Collaborator

JMS55 commented Jun 29, 2025

I'd rather it unwrap in wgpu, personally. I can't imagine why you would want it to be optional.

Users are likely going to perform a backend check higher-up, and switch between e.g. dlss_vk and dlss_dx.

@Wumpf Wumpf self-assigned this Jul 2, 2025
@Wumpf Wumpf self-requested a review July 2, 2025 15:23
@Wumpf
Copy link
Member

Wumpf commented Jul 6, 2025

Very strongly in favor of keeping options instead of wgpu-internal unwrap!
The check has to be done somewhere and passing the option on is preferable to an internal unwrap. Sure often there's a higher level check first. But there's also other situations where I may do a speculative as_hal and then don't act on it if it didn't work out.

And either way we need less unwraps in wgpu when we can avoid it! I never ever want to leave my users with an application crashing inside of wgpu because I didn't real all the details of a docstring warning about correct use.

Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

beautiful, huge improvement!
the locking mechanism is quite elaborate, I think I followed it all the way and there's no error in there but it would be nice to add some basic tests for this. Any chance we could add some test code that covers the guard types?

Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

we decided on maintainer call that this is good enough for release, but we still want some extra tests here

@cwfitzgerald cwfitzgerald enabled auto-merge (rebase) July 9, 2025 22:07
@cwfitzgerald cwfitzgerald merged commit 4f15635 into gfx-rs:trunk Jul 9, 2025
40 checks passed
@cwfitzgerald cwfitzgerald deleted the cw/as_hal_guards branch July 9, 2025 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants