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

[wgpu-hal] Upgrade to ash 0.38 #5504

Merged
merged 1 commit into from
May 13, 2024
Merged

Conversation

MarijnS95
Copy link
Contributor

https://github.com/ash-rs/ash/releases/tag/0.38.0

In this release a lot of breaking changes have been made: builder structs are dropped in favour of always having a lifetime parameter available on every raw Vulkan structure (when they contain one or more pointers). This massively contributes to lifetime (and mutable aliasing) safety, but requires significant changes to some uses in wgpu-hal.

All function pointer loaders for both Vulkan core and extensions have moved into the root of ash::, making ash::vk:: a more pure sys-like module. Extensions have their own ash::<prefix>::<ext> module to clearly separate and group their items from the core. Besides NAME and the usual *Fn wrappers, the handwritten extensions module is now only available via this path. This to combat the previous inconsistency between ash::KhrSomeExtFn::name() vs ash::extensions::khr::SomeExt::name().

The Vulkan core clearly splits functions across device and instance functions, to keep functions that can be loaded without a dispatch-table for a device via get_device_proc_addr() apart from instance functions. This concept has now been applied to extension functions, making it possible to load them optimized for a device too (when the shared table would previously include instance functions and require the whole thing to be loaded via get_instance_proc_addr()), and in the rare case of 3 hand-written extension wrappers: load instance functions via get_device_proc_addr() resulting in NULL pointers.

Finally, a few new helpers like _as_slice() and _as_c_str() are available (the former only for statically-sized struct-owned arrays with a length delimiter field) to simplify commonly written patterns.

Connections
Link to the issues addressed by this PR, or dependent PRs in other repositories
#5501

Description
Describe what problem this is solving, and how it's solved.
New crate release of ash that users will likely want to start using, especially since it exposes a many more Vulkan extensions (generated against a newer spec) and has significant lifetime-safety improvements and other usability-refactors.

Testing
Explain how this change is tested.
cargo r -p wgpu-examples for a few of them.

Checklist

  • Run cargo fmt.
  • Run cargo clippy. If applicable, add:
    • --target wasm32-unknown-unknown
    • --target wasm32-unknown-emscripten
  • Run cargo xtask test to run tests.
    This fails on the water test, but also on trunk. As seen when running cargo r -p wgpu-examples water, a lot of SYNC-HAZARD-WRITE-AFTER-WRITE validation issues are detected.
  • Add change to CHANGELOG.md. See simple instructions inside file.

I realize that this is a bit of a sloppy PR with some TODOs, which I'd like to discuss to find the cleanest way forward :)

@MarijnS95 MarijnS95 requested a review from a team as a code owner April 6, 2024 16:34
Cargo.toml Outdated Show resolved Hide resolved
wgpu-hal/src/vulkan/instance.rs Outdated Show resolved Hide resolved
wgpu-hal/src/vulkan/command.rs Outdated Show resolved Hide resolved
wgpu-hal/src/vulkan/adapter.rs Outdated Show resolved Hide resolved
wgpu-hal/src/vulkan/device.rs Outdated Show resolved Hide resolved
wgpu-hal/src/vulkan/adapter.rs Outdated Show resolved Hide resolved
wgpu-hal/src/vulkan/device.rs Outdated Show resolved Hide resolved
wgpu-hal/src/vulkan/device.rs Show resolved Hide resolved
wgpu-hal/src/vulkan/device.rs Outdated Show resolved Hide resolved
wgpu-hal/src/vulkan/device.rs Outdated Show resolved Hide resolved
wgpu-hal/src/vulkan/instance.rs Show resolved Hide resolved
wgpu-hal/src/vulkan/instance.rs Show resolved Hide resolved
wgpu-hal/src/vulkan/instance.rs Show resolved Hide resolved
wgpu-hal/src/vulkan/adapter.rs Outdated Show resolved Hide resolved
wgpu-hal/src/vulkan/adapter.rs Outdated Show resolved Hide resolved
@cwfitzgerald

This comment was marked as resolved.

teoxoy

This comment was marked as resolved.

@ErichDonGubler
Copy link
Member

@MarijnS95: Do you think you'll be able to attend to this soon? I'm excited to see quite a few duplicate dependencies like libloading 0.7 disappear once this lands. 🙂 I filed #5649 in anticipation. No pressure, though!

@MarijnS95

This comment was marked as resolved.

@cwfitzgerald

This comment was marked as resolved.

@MarijnS95

This comment was marked as resolved.

@jimblandy

This comment was marked as resolved.

@ErichDonGubler

This comment was marked as resolved.

@cwfitzgerald
Copy link
Member

cwfitzgerald commented May 8, 2024

I think once merge conflicts are resolved, it is ready to go

Copy link
Member

@teoxoy teoxoy left a comment

Choose a reason for hiding this comment

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

Looks good to me as well!

@MarijnS95 MarijnS95 requested a review from cwfitzgerald May 13, 2024 14:17
Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

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

CI is mad, but lets go :)

@MarijnS95
Copy link
Contributor Author

MarijnS95 commented May 13, 2024

@cwfitzgerald Maybe I broke that with the change in features for winapi? Though the libloaderapi feature that I moved around doesn't seem to impact whether errhandlingapi is enabled.

It is also a tad unfortunate that local cargo clippy on Rust 1.78 shows a bunch of unnecessary qualification warnings despite #5651 being merged, in code that wasn't touched in this PR.

@MarijnS95
Copy link
Contributor Author

The cause of this error is now obvious. With this PR there's no direct dependency on libloading 0.7 anymore (there is via glutin, but it doesn't seem to be enabled by default, or at least not for the default target(s)/crate(s)), which used to turn on errhandlingapi on winapi:

$ cargo tree -e features --target x86_64-pc-windows-msvc
...
│   │   ├── wgpu-hal v0.20.0 (/home/marijn/Code/TraverseResearch/wgpu/wgpu-hal)
...
│   │   │   ├── ash feature "default"
│   │   │   │   ├── ash v0.37.3+1.3.251
│   │   │   │   │   └── libloading feature "default"
│   │   │   │   │       └── libloading v0.7.4
│   │   │   │   │           ├── winapi feature "default" (*)
│   │   │   │   │           ├── winapi feature "errhandlingapi"
│   │   │   │   │           │   └── winapi v0.3.9
...

So we now have one more explicit feature dependency to list in Cargo.toml!

https://github.com/ash-rs/ash/releases/tag/0.38.0

In this release a lot of breaking changes have been made: builder
structs are dropped in favour of always having a lifetime parameter
available on every raw Vulkan structure (when they contain one or
more pointers).  This massively contributes to lifetime (and mutable
aliasing) safety, but requires significant changes to some uses in
`wgpu-hal`.

All function pointer loaders for both Vulkan core and extensions
have moved into the root of `ash::`, making `ash::vk::` a more pure
`sys`-like module.  Extensions have their own `ash::<prefix>::<ext>`
module to clearly separate and group their items from the core.  Besides
`NAME` and the usual `*Fn` wrappers, the handwritten `extensions` module
is now only available via this path.  This to combat the
previous inconsistency between `ash::KhrSomeExtFn::name()` vs
`ash::extensions::khr::SomeExt::name()`.

The Vulkan core clearly splits functions across `device` and `instance`
functions, to keep functions that can be loaded without a dispatch-table
for a device via `get_device_proc_addr()` apart from instance functions.
This concept has now been applied to extension functions, making it
possible to load them optimized for a device too (when the shared
table would previously include instance functions and require the whole
thing to be loaded via `get_instance_proc_addr()`), and in the rare
case of 3 hand-written extension wrappers: load instance functions via
`get_device_proc_addr()` resulting in `NULL` pointers.

Finally, a few new helpers like `_as_slice()` and `_as_c_str()` are
available (the former only for statically-sized struct-owned arrays with
a length delimiter field) to simplify commonly written patterns.
@cwfitzgerald
Copy link
Member

Thanks for the work on this and seeing this through to the end!

@cwfitzgerald cwfitzgerald merged commit 8879733 into gfx-rs:trunk May 13, 2024
25 checks passed
@MarijnS95 MarijnS95 deleted the ash-0.38 branch May 13, 2024 16:03
@MarijnS95
Copy link
Contributor Author

No worries, I hope you can all benefit/enjoy the incremental improvements in the latest ash release now!

EriKWDev pushed a commit to bitwise-git/wgpu that referenced this pull request May 14, 2024
EriKWDev pushed a commit to bitwise-git/wgpu that referenced this pull request May 14, 2024
jimblandy pushed a commit to jimblandy/wgpu that referenced this pull request May 23, 2024
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.

5 participants