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

Rework wgpu-rs Context #6619

Merged
merged 1 commit into from
Dec 4, 2024
Merged

Conversation

cwfitzgerald
Copy link
Member

@cwfitzgerald cwfitzgerald commented Nov 26, 2024

Connections

Closes #6324
Closes #6145

Description

Here it is. This reworks how the wgpu context works so that there is:

  • No overhead when there is more than one `wgpu backend.
  • Low cognitive overhead to the dynamic dispatch.
  • Two less copies of the api hanging around.
  • Split the dispatch trait from a single Context, to one trait per type of object. This reflects how we want our code to be structured. The fact everything is on the context is now a wgpu_context implementation impl.
  • Moves Drop code into the backends as opposed to being in the context.

Testing

Tested manually on WebGPU and native tests. Covered through CI. Added compile tests to make sure #6145 doesn't come back.

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.

#[cfg(wgpu_core)]
#[inline]
#[allow(unused)]
pub fn as_core(&self) -> &$core {
Copy link
Member

@Wumpf Wumpf Nov 26, 2024

Choose a reason for hiding this comment

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

Result would be nicer imho
depends a bit on the usecase tbf

Copy link
Member Author

@cwfitzgerald cwfitzgerald Nov 26, 2024

Choose a reason for hiding this comment

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

We'll get there later, no idea what the current behavior is if you mix context types, at least now it'll panic in a nice well defined place.

Comment on lines 477 to 482
pub enum $name {
#[cfg(wgpu_core)]
Core($core),
#[cfg(webgpu)]
WebGPU($webgpu),
}
Copy link
Contributor

Choose a reason for hiding this comment

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

So for #6330 we would need to add Custom(Arc<dyn $trait>) variant, but we would probably want to put it behind a cfg.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that's the idea.

Comment on lines +19 to +22
[[test]]
name = "wgpu-compile-test"
path = "compile_tests/root.rs"
harness = true
Copy link
Member Author

Choose a reason for hiding this comment

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

Lookie: Our test suite has grown a new feature. We can now ensure that a given bit of wgpu code does not compile. This is very useful to make sure that #6145 doesn't come back.

Comment on lines +722 to +749
crate::cmp::impl_eq_ord_hash_arc_address!(ContextWgpuCore => .0);
crate::cmp::impl_eq_ord_hash_proxy!(CoreAdapter => .id);
crate::cmp::impl_eq_ord_hash_proxy!(CoreDevice => .id);
crate::cmp::impl_eq_ord_hash_proxy!(CoreQueue => .id);
crate::cmp::impl_eq_ord_hash_proxy!(CoreShaderModule => .id);
crate::cmp::impl_eq_ord_hash_proxy!(CoreBindGroupLayout => .id);
crate::cmp::impl_eq_ord_hash_proxy!(CoreBindGroup => .id);
crate::cmp::impl_eq_ord_hash_proxy!(CoreTextureView => .id);
crate::cmp::impl_eq_ord_hash_proxy!(CoreSampler => .id);
crate::cmp::impl_eq_ord_hash_proxy!(CoreBuffer => .id);
crate::cmp::impl_eq_ord_hash_proxy!(CoreTexture => .id);
crate::cmp::impl_eq_ord_hash_proxy!(CoreBlas => .id);
crate::cmp::impl_eq_ord_hash_proxy!(CoreTlas => .id);
crate::cmp::impl_eq_ord_hash_proxy!(CoreQuerySet => .id);
crate::cmp::impl_eq_ord_hash_proxy!(CorePipelineLayout => .id);
crate::cmp::impl_eq_ord_hash_proxy!(CoreRenderPipeline => .id);
crate::cmp::impl_eq_ord_hash_proxy!(CoreComputePipeline => .id);
crate::cmp::impl_eq_ord_hash_proxy!(CorePipelineCache => .id);
crate::cmp::impl_eq_ord_hash_proxy!(CoreCommandEncoder => .id);
crate::cmp::impl_eq_ord_hash_proxy!(CoreComputePass => .id);
crate::cmp::impl_eq_ord_hash_proxy!(CoreRenderPass => .id);
crate::cmp::impl_eq_ord_hash_proxy!(CoreCommandBuffer => .id);
crate::cmp::impl_eq_ord_hash_proxy!(CoreRenderBundleEncoder => .id);
crate::cmp::impl_eq_ord_hash_proxy!(CoreRenderBundle => .id);
crate::cmp::impl_eq_ord_hash_proxy!(CoreSurface => .id);
crate::cmp::impl_eq_ord_hash_proxy!(CoreSurfaceOutputDetail => .surface_id);
crate::cmp::impl_eq_ord_hash_proxy!(CoreQueueWriteBuffer => .mapping.ptr);
crate::cmp::impl_eq_ord_hash_proxy!(CoreBufferMappedRange => .ptr);
Copy link
Member Author

Choose a reason for hiding this comment

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

Lookie: Now the backend types are responsible for implementing PartialEq etc how they see fit. Most of them use this proxy type, which just uses a single member for the partial eq behavior.

Copy link
Member

Choose a reason for hiding this comment

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

Once we get rid of the registries in wgpu-core this will be easy to adapt, just use impl_eq_ord_hash_arc_address. Nice design!

Copy link
Member Author

Choose a reason for hiding this comment

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

Lookie! This file is very important

@cwfitzgerald cwfitzgerald marked this pull request as ready for review December 4, 2024 02:19
@cwfitzgerald cwfitzgerald requested a review from a team as a code owner December 4, 2024 02:19
impl Identifier {
pub fn create() -> Self {
let id = NEXT_ID.fetch_add(1, Ordering::Relaxed);
// Safety: Will take 7000+ years of constant incrementing to overflow. It's fine.
Copy link
Member

Choose a reason for hiding this comment

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

😆

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.

Overall a really nice design! Much more straightforward than what we had before.

I tried to review it as best as I could, I didn't review code movement though since that would have been a nightmare.

@teoxoy
Copy link
Member

teoxoy commented Dec 4, 2024

There is something I was wondering about: Since the public wgpu types are now just:

pub struct X {
    pub(crate) inner: dispatch::DispatchX,
}

Wouldn't it be possible to simplify things further and use the DispatchX (renaming them to X) types for the public API instead?

@Wumpf Wumpf self-requested a review December 4, 2024 11:17
@cwfitzgerald cwfitzgerald merged commit 4e139ed into gfx-rs:trunk Dec 4, 2024
27 checks passed
@cwfitzgerald cwfitzgerald deleted the cw/ctx-rework-2 branch December 4, 2024 17:00
@sagudev sagudev mentioned this pull request Dec 4, 2024
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants