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 needs a better (and stable) public API #6245

Open
SludgePhD opened this issue Sep 9, 2024 · 15 comments
Open

wgpu needs a better (and stable) public API #6245

SludgePhD opened this issue Sep 9, 2024 · 15 comments

Comments

@SludgePhD
Copy link
Contributor

I maintain 5 projects, some public and some private, that depend (either transitively or directly) on wgpu. Its API has caused a few problems for me that I'd like to document here, in hope that a future release can address them.

The fact that the API is essentially completely unstable and changes in small but breaking changes with every single release means:

  • The breaking changes create a lot of work for me and other folks in similar positions, who have to migrate to every new release
  • wgpu's position as a "vocabulary crate", where crates may want to use its types in their public APIs, means that upgrades between major versions can have a cascading effect throughout the ecosystem, where crates are blocked from upgrading until all of their dependencies are on a newer wgpu version
  • Using multiple major versions of wgpu causes linker errors #3105 means that the whole crate graph can only include a single wgpu version even if the previous point isn't already an issue

At the same time, wgpu's API is a direct port of the JavaScript WebGPU API, but without any of the affordances enabled by JavaScript as a language, which are what make this API work pretty nicely in JavaScript. This choice introduces the following problems in the wgpu Rust API:

  • Rust has no anonymous records, so everything takes a CumbersomeNameDescriptor as an argument, which has to be separately imported and needs to be written again and again at every call site
  • Rust has no concept of optional function arguments or fields, so every descriptor has to be filled in with ..Default::default() in the best case, or every single field has to be manually specified in the worst case (for example, for RenderPipelineDescriptor, ComputePipelineDescriptor, TextureDescriptor, BufferDescriptor, ...)
  • The factory method pattern, where every object is created by calling a method on Device, makes it unclear how to obtain an instance of something without further hand-written documentation (which, thankfully, is present here), is unfamiliar since it's rarely seen in Rust libraries, and makes the common pattern of having multiple constructor methods on an object (think Vec::new and Vec::with_capacity) more costly, since Device is already overflowing with factory methods.

The API design issues could be addressed by writing a Rusty API for WebGPU, something that heavily uses established patterns from the Rust-world, like builders and multiple constructor methods, as alternatives to optional parameters. Having a Buffer::builder(device) method would also obviate the need to import and name a dozen different SpecificThingDescriptor structs every time an object is created.

(mind you, this isn't going to be an easy task, and I don't have the time to volunteer to work on it. I mostly just wanted to document my frustrations so that other potential users can make a more informed decision when considering wgpu as their graphics API of choice)

@ErichDonGubler
Copy link
Member

This has been brought up in a few places where we communicate with users. I'm going to record the position that has seemed to have consensus before.

With the upstream WebGPU API not being fully stable itself, it's hard to promise that we won't be unstable for now. Because there's still several places where we aren't spec-compliant, it's also hard to promise a stable API. We do, however, think that a stable API is interesting, and is a certainty something we want to commit to once we've proven that we're spec-compliant.

@ErichDonGubler
Copy link
Member

ErichDonGubler commented Sep 10, 2024

Just this last weekend, I was playing with applying derived builder APIs, because I was tired of not being able to write code roughly analagous to the JS API. I think the following examples from this WIP (unofficial) PR were salient in how much easier we could make it to write WGPU code: erichdongubler-mozilla#96

Cherry-picking the diff from benches/benchers/renderpass.rs:

diff --git a/benches/benches/renderpass.rs b/benches/benches/renderpass.rs
index fe23aa62be..10b54b6819 100644
--- a/benches/benches/renderpass.rs
+++ b/benches/benches/renderpass.rs
@@ -85,22 +85,14 @@
 
         let mut texture_views = Vec::with_capacity(texture_count);
         for i in 0..texture_count {
-            let texture = device_state
-                .device
-                .create_texture(&wgpu::TextureDescriptor {
-                    label: Some(&format!("Texture {i}")),
-                    size: wgpu::Extent3d {
-                        width: 1,
-                        height: 1,
-                        depth_or_array_layers: 1,
-                    },
-                    mip_level_count: 1,
-                    sample_count: 1,
-                    dimension: wgpu::TextureDimension::D2,
-                    format: wgpu::TextureFormat::Rgba8UnormSrgb,
-                    usage: wgpu::TextureUsages::TEXTURE_BINDING,
-                    view_formats: &[],
-                });
+            let texture = device_state.device.create_texture(
+                &wgpu::TextureDescriptor::builder()
+                    .label(&*format!("Texture {i}"))
+                    .size(Default::default())
+                    .format(wgpu::TextureFormat::Rgba8UnormSrgb)
+                    .usage(wgpu::TextureUsages::TEXTURE_BINDING)
+                    .build(),
+            );
             texture_views.push(texture.create_view(&wgpu::TextureViewDescriptor {
                 label: Some(&format!("Texture View {i}")),
                 ..Default::default()
@@ -138,14 +130,11 @@
             .device
             .create_shader_module(wgpu::include_wgsl!("renderpass.wgsl"));
 
-        let pipeline_layout =
-            device_state
-                .device
-                .create_pipeline_layout(&wgpu::PipelineLayoutDescriptor {
-                    label: None,
-                    bind_group_layouts: &[&bind_group_layout],
-                    push_constant_ranges: &[],
-                });
+        let pipeline_layout = device_state.device.create_pipeline_layout(
+            &wgpu::PipelineLayoutDescriptor::builder()
+                .bind_group_layouts(&[&bind_group_layout])
+                .build(),
+        );
 
         let mut vertex_buffers = Vec::with_capacity(vertex_buffer_count);
         for _ in 0..vertex_buffer_count {
@@ -183,59 +172,44 @@
             });
         }
 
-        let pipeline =
-            device_state
-                .device
-                .create_render_pipeline(&wgpu::RenderPipelineDescriptor {
-                    label: None,
-                    layout: Some(&pipeline_layout),
-                    vertex: wgpu::VertexState {
-                        module: &sm,
-                        entry_point: Some("vs_main"),
-                        buffers: &vertex_buffer_layouts,
-                        compilation_options: wgpu::PipelineCompilationOptions::default(),
-                    },
-                    primitive: wgpu::PrimitiveState {
-                        topology: wgpu::PrimitiveTopology::TriangleList,
-                        strip_index_format: None,
-                        front_face: wgpu::FrontFace::Cw,
-                        cull_mode: Some(wgpu::Face::Back),
-                        polygon_mode: wgpu::PolygonMode::Fill,
-                        unclipped_depth: false,
-                        conservative: false,
-                    },
-                    depth_stencil: None,
-                    multisample: wgpu::MultisampleState::default(),
-                    fragment: Some(wgpu::FragmentState {
-                        module: &sm,
-                        entry_point: Some("fs_main"),
-                        targets: &[Some(wgpu::ColorTargetState {
-                            format: wgpu::TextureFormat::Rgba8UnormSrgb,
-                            blend: None,
-                            write_mask: wgpu::ColorWrites::ALL,
-                        })],
-                        compilation_options: wgpu::PipelineCompilationOptions::default(),
-                    }),
-                    multiview: None,
-                    cache: None,
-                });
+        let pipeline = device_state.device.create_render_pipeline(
+            &wgpu::RenderPipelineDescriptor::builder()
+                .layout(&pipeline_layout)
+                .vertex(
+                    wgpu::VertexState::from_module(&sm)
+                        .entry_point("vs_main")
+                        .buffers(&vertex_buffer_layouts)
+                        .build(),
+                )
+                .primitive(
+                    wgpu::PrimitiveState::builder()
+                        .front_face(wgpu::FrontFace::Cw)
+                        .cull_mode(wgpu::Face::Back)
+                        .build(),
+                )
+                .fragment(
+                    wgpu::FragmentState::from_module(&sm)
+                        .entry_point("fs_main")
+                        .targets(&[Some(
+                            wgpu::ColorTargetState::builder()
+                                .format(wgpu::TextureFormat::Rgba8UnormSrgb)
+                                .build(),
+                        )])
+                        .build(),
+                )
+                .build(),
+        );
 
         let render_target = device_state
             .device
-            .create_texture(&wgpu::TextureDescriptor {
-                label: Some("Render Target"),
-                size: wgpu::Extent3d {
-                    width: 1,
-                    height: 1,
-                    depth_or_array_layers: 1,
-                },
-                mip_level_count: 1,
-                sample_count: 1,
-                dimension: wgpu::TextureDimension::D2,
-                format: wgpu::TextureFormat::Rgba8UnormSrgb,
-                usage: wgpu::TextureUsages::RENDER_ATTACHMENT,
-                view_formats: &[],
-            })
+            .create_texture(
+                &wgpu::TextureDescriptor::builder()
+                    .label("Render Target")
+                    .size(Default::default())
+                    .format(wgpu::TextureFormat::Rgba8UnormSrgb)
+                    .usage(wgpu::TextureUsages::RENDER_ATTACHMENT)
+                    .build(),
+            )
             .create_view(&wgpu::TextureViewDescriptor::default());
 
         let mut bindless_bind_group = None;
@@ -274,50 +248,41 @@
                 .device
                 .create_shader_module(wgpu::include_wgsl!("renderpass-bindless.wgsl"));
 
-            let bindless_pipeline_layout =
-                device_state
-                    .device
-                    .create_pipeline_layout(&wgpu::PipelineLayoutDescriptor {
-                        label: None,
-                        bind_group_layouts: &[&bindless_bind_group_layout],
-                        push_constant_ranges: &[],
-                    });
+            let bindless_pipeline_layout = device_state.device.create_pipeline_layout(
+                &wgpu::PipelineLayoutDescriptor::builder()
+                    .bind_group_layouts(&[&bindless_bind_group_layout])
+                    .build(),
+            );
 
-            bindless_pipeline = Some(device_state.device.create_render_pipeline(
-                &wgpu::RenderPipelineDescriptor {
-                    label: None,
-                    layout: Some(&bindless_pipeline_layout),
-                    vertex: wgpu::VertexState {
-                        module: &bindless_shader_module,
-                        entry_point: Some("vs_main"),
-                        buffers: &vertex_buffer_layouts,
-                        compilation_options: wgpu::PipelineCompilationOptions::default(),
-                    },
-                    primitive: wgpu::PrimitiveState {
-                        topology: wgpu::PrimitiveTopology::TriangleList,
-                        strip_index_format: None,
-                        front_face: wgpu::FrontFace::Cw,
-                        cull_mode: Some(wgpu::Face::Back),
-                        polygon_mode: wgpu::PolygonMode::Fill,
-                        unclipped_depth: false,
-                        conservative: false,
-                    },
-                    depth_stencil: None,
-                    multisample: wgpu::MultisampleState::default(),
-                    fragment: Some(wgpu::FragmentState {
-                        module: &bindless_shader_module,
-                        entry_point: Some("fs_main"),
-                        targets: &[Some(wgpu::ColorTargetState {
-                            format: wgpu::TextureFormat::Rgba8UnormSrgb,
-                            blend: None,
-                            write_mask: wgpu::ColorWrites::ALL,
-                        })],
-                        compilation_options: wgpu::PipelineCompilationOptions::default(),
-                    }),
-                    multiview: None,
-                    cache: None,
-                },
-            ));
+            bindless_pipeline = Some(
+                device_state.device.create_render_pipeline(
+                    &wgpu::RenderPipelineDescriptor::builder()
+                        .layout(&bindless_pipeline_layout)
+                        .vertex(
+                            wgpu::VertexState::from_module(&bindless_shader_module)
+                                .entry_point("vs_main")
+                                .buffers(&vertex_buffer_layouts)
+                                .build(),
+                        )
+                        .primitive(
+                            wgpu::PrimitiveState::builder()
+                                .front_face(wgpu::FrontFace::Cw)
+                                .cull_mode(wgpu::Face::Back)
+                                .build(),
+                        )
+                        .fragment(
+                            wgpu::FragmentState::from_module(&bindless_shader_module)
+                                .entry_point("fs_main")
+                                .targets(&[Some(
+                                    wgpu::ColorTargetState::builder()
+                                        .format(wgpu::TextureFormat::Rgba8UnormSrgb)
+                                        .build(),
+                                )])
+                                .build(),
+                        )
+                        .build(),
+                ),
+            );
         }
 
         Self {

I think builders are helpful with the above—builder API usage feels far easier to read and write with complicated structures like WGPU's. While builder APIs (as I implemented them above) don't relieve the Rust-specific onus of naming descriptor types, it does allow users to ergonomically name only the fields they intend to specify. This is more forwards-compatible code when we add fields, as mentioned in the OP, reaping some of the benefits of JS API not breaking code when new optional fields are adding.

CC @gfx-rs/wgpu.

@Wumpf
Copy link
Member

Wumpf commented Sep 10, 2024

Interesting, didn't know about bon! It has runtime perf very well documented, but haven't found anything about compile time performance. It uses a lot of generic types for its runtime type safety which makes me wary

@Sxmourai
Copy link

I also think the API could be better, for example wgpu::Buffer holds a map context, and the user has no way to check if the buffer is already mapped, functions don't return Results but crashes (unmap, map_async, get_mapped_range...). That's just a small example, but I've seen a lot of other things that make no sense in the API (I hate the *Descriptor pattern too).

@grovesNL
Copy link
Collaborator

@Sxmourai wgpu intentionally doesn't return Result because of how errors are handled and to match the behavior from the WebGPU specification. wgpu's error handling needs better documentation (#3767) but in general you can control whether the default error handler causes a crash or not.

@grovesNL
Copy link
Collaborator

For what it's worth, I like the explicitness of the descriptor API when working with graphics code and how the descriptors directly match the web API. I think we'd lose that if we only supported a builder API.

@Sxmourai
Copy link

Okay, so maybe a fork that would try to return Results and provide a more rusty api would be more appreciated ?
Also I guess for big projects it's good, but when doing quick prototyping, it's a pain to write (it's less verbose than Vulkan, but I find it far from perfect)
Also changing the default error handler is not a straight-forward process, and also very verbose for absolutely no reason... I don't want to do "try... catch" but in a poor way

@grovesNL
Copy link
Collaborator

@Sxmourai

Also changing the default error handler is not a straight-forward process, and also very verbose for absolutely no reason

Setting the default error handler should be as simple as passing a callback to https://docs.rs/wgpu/latest/wgpu/struct.Device.html#method.on_uncaptured_error Is there something else you'd expect to see there instead?

@Sxmourai
Copy link

Well adding a custom error handler, that matches a special error type every time is harder than just doing a match statement on the return value, but yeah i've seen how to use the on_captured_error

@Wumpf
Copy link
Member

Wumpf commented Sep 10, 2024

Okay, so maybe a fork that would try to return Results and provide a more rusty api would be more appreciated

Your fork pretty much has to leave out WebGPU support then because almost all webgpu errors are asynchronous. One could write a new interface to wgpu-core where most things are synchronous results, but it wouldn't go all the way either. (that would surely be an interesting and much more tractable project btw.. A sort of "native only" version of wgpu)
In any case this is almost certainly out of scope of the wgpu project which aims to provide a portable WebGPU interface, see mission

@Sxmourai
Copy link

And returning an "AsyncResult", that you could use like: unwrap_block which waits to get the result, check if there is any error and returns/crashs
And then unwrap_async(callback) which calls the callback when the function is done ?

Also some wgpu errors are async, but the errors I'm complaining about on wgpu::Buffer are not, for example unmapping a buffer that isn't mapped... This will crash, but It could return a result, it is not async !

@Wumpf
Copy link
Member

Wumpf commented Sep 11, 2024

And returning an "AsyncResult",

That would still be not a Result Rust type so you can't propagate it like a Rust result type, making this whole thing a lot less useful imho. I'd also be worried about the overhead that the additional javascript calls for error scopes around every single method would imply.

unwrap_block which waits to get the result,

You can't just block in the browser in the flow of your code, you have to yield back to the browser.

.. that said I'd love to see some practical experimentation in this area :). I'm just very skeptical ;)

unmapping a buffer that isn't mapped... This will crash, but It could return a result, it is not async !

If it crashes when a custom error handler is installed that's a bug. All crashes with a custom error handler are!
Otherwise again same story, we can't return an error here because the WebGPU spec doesn't https://www.w3.org/TR/webgpu/#dom-gpubuffer-unmap
..in fact unmapping a buffer that isn't mapped is not even supposed to be a failure, so sounds like there's a bug regardless, can you file an issue ideally with a short repro? Thanks!

@Sxmourai
Copy link

e.g. just call unmap twice on a buffer, and get:

Caused by:
    In Buffer::buffer_unmap
    Buffer is not mapped

But the point isn't this particular use case, I think the API could greatly be improved, If this crate really wants to be exactly following the WebGPU standard then maybe make another crate that would be more rusty ? Also maybe we should check if there are any async runtimes/libs that could have some solutions for the async errors problem...

We could have "commands" for error handling called when executing the command... For example on the user side you have the "unwrap, expect, and other functions like map_err" which would get called when the error is detected... But we need a special type of result, because we have immediate error (on validation of commands) and async errors (which are detected on queue.submit or other ? I'm definitely not a wgpu expert)

I want to try different error handling methods, but the API changes are too big for a single (15 years old) guy...

@Sxmourai
Copy link

I was adding different helper functions and I can't stop wandering:
wgpu::backend::wgpu_core::ContextWgpuCore has nearly all functions that return results, and then the wgpu_core::Context just calls "unwrap" on it (it just checks if err and throws error)... Why ?
Why not expose the good API with try_* and then keep old functions that basically call .unwrap on all try_* functions ?

@caelunshun
Copy link

@Sxmourai wgpu-core isn't used on the WebGPU backend. WebGPU errors are always asynchronous, so your proposed try_* functions wouldn't be implementable.

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

No branches or pull requests

6 participants