Add buffer debug labels #528

aloucks · 2020-03-20T06:40:58Z

Related #404

@kvark Is this what you had in mind? The webgpu headers have labels on all the descriptor structs and I didn't see any specific functions for altering/adding labels after creation. I'll add the rest if this looks good to you.

I also went looking for command buffer marker support in gfx-hal so that I could add group labels and markers. Is this functionality absent in hal or did I miss it?

kvark · 2020-03-20T16:08:14Z

Yes, this is what I had in mind.
Also, the upstream API has getters and setters for the labels, but it's fine if we get that later. At least propagating the one in the descriptor is already very useful.

Is this functionality absent in hal or did I miss it?

Correct, they are absent today. Should be a part of gfx-rs/gfx#1508
Do you want to try to add them?

aloucks · 2020-03-20T23:46:42Z

@kvark

I've added debug labels for the items that hal supports and are exposed. I ran the triangle example in RenderDoc and verified that the command encoder label was applied.

A few observations:

It's frustrating that cargo fmt doesn't just work for this project.
- It's using unstable rustfmt flags that require nightly.
- The newline setting is Native which blows up on windows if you have git configured to use LF by default (which I do). Auto would be preferred. If Native is required for some other purpose, there should be a corresponding .gitattributes that also enforces native EOL format (ideally both should exist and be in harmony anyway). Alternatively, set both rustfmt.toml and .gitattributes config to always use LF.
- cargo fmt --check --all should be enforced via CI. I backed out the rustfmt changes because there were many other changes unrelated to what I had worked on.
It would be nice if GLFW was inlined as a submodule and configured to build statically so that building the examples "just worked" with minimal fatigue.
I don't think the descriptors should be shared with wgpu-rs. I think the enums and bitflags are fine, but eventually all/most of the descriptors are going to to have a pointers in them that don't always map well (cbindgen didn't like &str - Maybe there is a way to fix this?).
EDIT: It might also be worthwhile to have the set_object_name and friends functions take in a name that is generic (or just use CStr) rather than &str. I think we'll end up with converting back and forth and the current implementation imposes a maximum length. Also, I think the current implementation will panic if the name is >64 characters.

kvark · 2020-03-21T00:24:23Z

It's using unstable rustfmt flags that require nightly.

As far as I'm aware, using nightly rustfmt is not uncommon at all. You just do "cargo +nightly install rustfmt-nightly -f" and the rest is the same.

Auto would be preferred.

Sounds good!

cargo fmt --check --all should be enforced via CI. I backed out the rustfmt changes because there were many other changes unrelated to what I had worked on.

I disagree. It's annoying to have your CI failed (and everybody losing time blocked on that) simply because there was a piece of formatting that rustfmt didn't like.

It would be nice if GLFW was inlined as a submodule and configured to build statically so that building the examples "just worked" with minimal fatigue.

what kind of fatigue did you experience?

I don't think the descriptors should be shared with wgpu-rs

Unfortunately, now with the labels there isn't another way. So yes, we'll need to duplicate all the descriptors on the Rust side.

kvark · 2020-03-21T00:31:48Z

wgpu-core/src/device/mod.rs

+            ).unwrap();
+            if !desc.label.is_null() {
+                let label = ffi::CStr::from_ptr(desc.label).to_string_lossy();
+                self.raw.set_image_name(&mut image, &label);


hmm, that seems suboptimal. So we are getting *const char, then converting to &str to pass to gfx-rs, and then it probably converts it back to *const char at some point later on... Hmm.
At the same time, this is object creation, so it's not a big concern. Just that it would affect the API.
One way to address this (I'm not sure that we should, but still) is to have something like this:

enum Stringy<'a> { Rust(&'a str), C(&'a CStr), }

Yep - that's the issue I mentioned in the EDIT above.

I think gfx-hal should just take CStr and be done with it. It's purpose is to be low level.

problem is that not all the backends agree on the strings they want. D3D, GL, and Vulkan do agree, but Metal wants NSString anyway, so it doesn't want CStr.

You can convert a CStr to a &str by finding the length and creating the slice. You can't go in the other direction without allocating or having an intermediate buffer.

It's a copy and convert step, just like one would do to convert &str to NSString, or &str to CStr. In that sense, CStr doesn't have an advantage.

Isn't there a copy with &str to NSString anyway?

Wont &str -> &CStr -> &str -> [&CStr or NSString] always be more copies than
&str -> [&CStr or NSString].

I don't think anybody proposed &str -> &CStr -> &str -> [&CStr or NSString]

&str -> &CStr -> &str -> [&CStr or NSString]

This is what would currently happen with wgpu-rs. The label comes in as a &str from the rust descriptor. Then converted to a *const c_char to use with wgpu-native descriptor. Then converted back to &str to pass to gfx-hal. Then gfx-hal converts it back to CStr to use with native API (or NSString in the mac case).

Apologies, I'm using CStr and *const c_char interchangeably here to indicate a copy to null-terminated buffer.

I see, yeah. This isn't intended. As outlined in gfx-rs/wgpu-rs#156 , the main critical paths will be via wgpu-core (which works with &str) or websys, which needs its own strings anyway. Going via wgpu-native C API is kinda silly, only needed for compatibility with C apps and Dawn.

aloucks · 2020-03-21T00:42:05Z

As far as I'm aware, using nightly rustfmt is not uncommon at all. You just do "cargo +nightly install rustfmt-nightly -f" and the rest is the same.

I would argue that most public projects are not using nightly rustfmt / unstable options due to the friction that it causes. For example, winit switched to stable options after there was a constant flow of noisy PRs and churn with formatting flipping back and forth.

I disagree. It's annoying to have your CI failed (and everybody losing time blocked on that) simply because there was a piece of formatting that rustfmt didn't like.

This is easily overcome by making the format check the first step of the CI job. It fails fast, you run cargo fmt and push another commit. There is minimal overhead and time loss. With the formatting enforced, you don't have to worry about adding a bunch of noise in your PRs because the only code that will change is code that you modified.

I don't want to run rustfmt on individual files and then manually back out changes that are unrelated. I want to just run cargo fmt and be done with it. Having to perform fine grained rustfmt with manual changes is much more of a time sink than pushing an extra commit after a failed CI run.

aloucks · 2020-03-21T00:49:41Z

what kind of fatigue did you experience?

It was mostly fumbling with cmake and building GLFW out of band (and then remembering that you have to actually build the install target - just building the lib isn't good enough) and then setting up examples to use that install path. If I were on linux I think it would just use pkg-config and just work out of the box, but I'm primarily on Windows. Mostly, I've just become spoiled by the ease of use of cargo :)

kvark · 2020-03-21T01:06:19Z

I want to just run cargo fmt and be done with it. Having to perform fine grained rustfmt with manual changes is much more of a time sink than pushing an extra commit after a failed CI run.

First of all, with the current flow you do not have to do any formatting at all. Less work for you, less worries about.
If you do want to format, just make it a separate commit, done. No problem to merge that.

kvark · 2020-03-24T23:19:51Z

@aloucks we had a conversation about formatting of the project with a Gecko engineer and agreed to format this with an empty configuration on rustfmt stable. Let's not do any formatting in this PR and then follow up with all-format PR once the dust settles with the open PRs, ok?

aloucks · 2020-03-24T23:21:08Z

cool

aloucks · 2020-03-24T23:21:33Z

Can we do the same with wgpu-rs ?

kvark · 2020-03-24T23:22:59Z

The wgpu-rs doesn't have that requirement of being consistent with Gecko code, so it's still on the table. Let's not rush this just yet.

- bind group - bind group layout - command encoder - texture

aloucks · 2020-03-24T23:46:22Z

@kvark I'v rebased and fixed merge conflicts. I'd like to tackle the command encoder push/pop labels separately. With that said, merging this will require all of the affected descriptor types to be duplicated in wgpu-rs.

kvark · 2020-03-25T00:23:58Z

@aloucks thank you!
I was taking the last look, and I became more concerned about *const char here. If we expose *const char in wgpu-core, then:

(pro) wgpu-native and wgpu-remote don't need to have their own structures, they re-use the ones from wgpu-core
(con) wgpu-rs needs to have a separate Rusty structure
(con) any labels set from wgpu-rs need to go from &str -> *const char -> &str -> something which is quite sad...

Not sure what do do here. One "solution" is to convert gfx-rs to accept *const char for all labels. Another is more involved - having wgpu-core truly work with Rusty structures, and then have something on top for raw pointers in wgpu-native/wgpu-remote.
@grovesNL @msiglreith

aloucks · 2020-03-25T00:44:44Z

As much as I dislike duplication of code, I think it's appropriate in this case. I think all of the C FFI structures should be completely separate from the Rust ones. Right now it's confusing with the current layout and how things are re-exported in different directions. It's not always obvious if something is "raw" or the Rust variant until you see how it's used in other places (unless if course it has a pointer in it). The enums and bitflags can probably be shared but I think there's benefit in having a clean separation of the rest.

One "solution" is to convert gfx-rs to accept *const char for all labels.

This is what I was arguing for above! :) I don't particularly care if it's CStr vs *const c_char - my intent was just "raw c-style null terminated string".

grovesNL · 2020-03-25T00:58:30Z

@kvark IMO multiple string conversions for debug labels isn't too much of an issue so it's mostly just a question of which API we'd like to work with at each level. Although it does seem pretty reasonable to accept &CStr in gfx-hal (instead of &str) if we do it consistently throughout.

kvark · 2020-03-25T04:29:42Z

@grovesNL I'm not worried about object labels (this PR) as much as the markers. I imagine it being useful to just mostly have markers enabled (i.e. both debug and release builds, just not fully optimized shipping ones). And having a few heap allocations on every marker can have a devastating effect during command encoding.

aloucks · 2020-03-25T12:40:18Z

@kvark I've got an idea with flexible inlined strings. I'll update the PR later this evening.

kvark · 2020-03-25T23:18:15Z

I would argue that most public projects are not using nightly rustfmt / unstable options due to the friction that it causes. For example, winit switched to stable options after there was a constant flow of noisy PRs and churn with formatting flipping back and forth.

Wanted to follow up on that with this little quote from rustfmt itself:

graphics35334:gfx-memory dmalyshau$ cargo fmt
This version of rustfmt is deprecated. Use rustfmt-nightly. Use --force to run deprecated rustfmt.

Rustfmt stable is not a valid choice, it's deprecated.

aloucks · 2020-03-25T23:26:04Z

Rustfmt stable is not a valid choice, it's deprecated.

I'm fairly certain that it's not deprecated. I think you get that error if you installed rustfmt from cargo install rustfmt a long time ago before it was added as a default component with the rustup install.

kvark · 2020-03-25T23:27:35Z

Interesting. I'm currently trying to get it running from rustup component instead. Didn't realize the cargo binary could be outdated.

aloucks · 2020-03-25T23:27:42Z

$ rustup component list | grep installed
cargo-x86_64-pc-windows-msvc (installed)
clippy-x86_64-pc-windows-msvc (installed)
rust-analysis-x86_64-pc-windows-msvc (installed)
rust-docs-x86_64-pc-windows-msvc (installed)
rust-src (installed)
rust-std-wasm32-unknown-unknown (installed)
rust-std-x86_64-pc-windows-msvc (installed)
rustc-x86_64-pc-windows-msvc (installed)
rustfmt-x86_64-pc-windows-msvc (installed)

$ rustup --version
rustup 1.21.1 (7832b2ebe 2019-12-20)

$ rustc --version
rustc 1.42.0 (b8cedc004 2020-03-09)

$ cargo --version
cargo 1.42.0 (86334295e 2020-01-31)

kvark · 2020-03-25T23:32:23Z

Silly question: how are you running it from the installed component? I see that the binaries are installed in ~/.rustup/toolchains/stable-x86_64-apple-dawin/bin. It's not in my PATH, and I don't think it's supposed to be (i.e. I'd expect it to work based on the current toolchain override). So running cargo fmt is unable to see it installed. Trying to run rustup run stable cargo-fmt sees cargo-fmt but fails to find rustfmt.

aloucks · 2020-03-25T23:32:36Z

If you look in ~/.cargo/bin, the rustup, cargo, rustfmt, rls, rustdoc, and cargo-clippy binaries, should all be the exact same binary (on windows -- they might be symlinks on linux?) and they are all rustup. rustup masquerades itself at the other binaries (which is how the +nightly syntax works).

aloucks · 2020-03-25T23:35:10Z

~/.rustup/toolchains/stable-x86_64-apple-dawin/bin -- nope that should not be on the path -- only ~/.cargo/bin should be on the path.

kvark · 2020-03-26T00:02:38Z

Got the binaries after rustup update now. We should use the stable rustfmt. Thank you for clarifying this @aloucks !

aloucks · 2020-03-26T02:56:52Z

@kvark I've pushed a new commit with inlinable labels. This works for now but I think long/medium term this needs to move to gfx-rs.

This is the course of action I think we should take:

Update/add wrapper structs in wgpu-rs for all descriptors with labels
Update gfx-hal to accept *const c_char or &CStr for all label functions (I think &CStr is a better choice but ultimately they're both zero-cost and interchangeable)
- Move the inlinable Label struct and functionality to gfx-rs
Update wgpu to make use of the new function signatures and remove the Label struct here.
Add update push/pop group labels in gfx-rs (I think I saw PRs on this already but I can't remember if it's working yet)

kvark · 2020-03-26T22:48:50Z

wgpu-core/src/device/mod.rs

@@ -321,6 +321,12 @@ impl<B: GfxBackend> Device<B> {
        };

        let mut buffer = unsafe { self.raw.create_buffer(desc.size, usage).unwrap() };
+        let label = conv::label_from_ptr(desc.label);


I was just browsing the docs now, and noticed that to_string_lossy just borrows the existing contents when the string is a valid UTF8. I think that's all we really need here?
Do you think we could remove all the Label stuff and just call to_string_lossy on them before passing to gfx-hal?

Or even just to_str and log::warn on being unable to get Ok() from it (ignoring the label in this case)

Do you think we could remove all the Label stuff and just call to_string_lossy on them before passing to gfx-hal?

This is what I had before.

Deeply sorry if I missed that somehow! Could you link to that comment please?
I somehow assumed that all the conversion involve heap allocations before...

fdcf9e7

Maybe we should just hold off on this until gfx-hal is updated? The inlinable label stuff should be there anyway, as I described above.

I'm not quite feeling comfortable with having Label in gfx-hal at this point. Maybe I need to grow to it? :)
Also, gfx-hal was just released, and we are updating wgpu to use it (#537). We typically try to stay on the published version of gfx, since otherwise it's harder to patch things up.

Let's proceed with your original code? (I slightly prefer to_str with warning on failure, but we can change that later)

@kvark I rolled off the last commit and force pushed.

The inlinable label is left here if there's any interest in it later: https://github.com/aloucks/wgpu/tree/buffer_label_with_inline_string

kvark · 2020-03-27T00:09:27Z

wgpu-types/src/lib.rs

+
+impl Default for CommandEncoderDescriptor {
+    fn default() -> CommandEncoderDescriptor {
+        unsafe { std::mem::zeroed() }


why use zeroed instead of just assigning ptr::null() safely?

I can't remember at this point, but mem::zeroed is safe in this context. All zeros is a valid bit pattern for CommandEncoderDescriptor as it exists in this PR. However, I could see how someone could accidentally make it UB later (e.g. if for some reason a reference field was added).

I'll change it to use ptr::null.

kvark · 2020-03-27T00:27:37Z

bors r+

bors · 2020-03-27T01:22:31Z

Build succeeded

continuous-integration/travis-ci/push

kvark reviewed Mar 21, 2020

View reviewed changes

aloucks marked this pull request as ready for review March 21, 2020 00:54

kvark mentioned this pull request Mar 21, 2020

Basic debug markers API gfx-rs/gfx#3169

Merged

4 tasks

aloucks force-pushed the buffer_label branch from 3f6e7be to e385e30 Compare March 24, 2020 23:34

aloucks added 2 commits March 24, 2020 19:38

Add buffer debug labels

2700d1c

Add inital debug labels

fdcf9e7

- bind group - bind group layout - command encoder - texture

aloucks force-pushed the buffer_label branch from e385e30 to fdcf9e7 Compare March 24, 2020 23:43

kvark requested changes Mar 26, 2020

View reviewed changes

aloucks force-pushed the buffer_label branch from ca02eba to fdcf9e7 Compare March 26, 2020 23:34

kvark approved these changes Mar 27, 2020

View reviewed changes

Use ptr::null for default label

d64670a

bors bot merged commit 86981d9 into gfx-rs:master Mar 27, 2020

Add buffer debug labels #528

Add buffer debug labels #528

Conversation

aloucks commented Mar 20, 2020 • edited Loading

kvark commented Mar 20, 2020

aloucks commented Mar 20, 2020 • edited Loading

kvark commented Mar 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aloucks commented Mar 21, 2020

aloucks commented Mar 21, 2020

kvark commented Mar 21, 2020

kvark commented Mar 24, 2020

aloucks commented Mar 24, 2020

aloucks commented Mar 24, 2020

kvark commented Mar 24, 2020

aloucks commented Mar 24, 2020 • edited Loading

kvark commented Mar 25, 2020

aloucks commented Mar 25, 2020

grovesNL commented Mar 25, 2020

kvark commented Mar 25, 2020

aloucks commented Mar 25, 2020 • edited Loading

kvark commented Mar 25, 2020

aloucks commented Mar 25, 2020

kvark commented Mar 25, 2020

aloucks commented Mar 25, 2020 • edited Loading

kvark commented Mar 25, 2020

aloucks commented Mar 25, 2020 • edited Loading

aloucks commented Mar 25, 2020

kvark commented Mar 26, 2020

aloucks commented Mar 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aloucks Mar 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kvark commented Mar 27, 2020

bors bot commented Mar 27, 2020

Build succeeded

aloucks commented Mar 20, 2020 •

edited

Loading

aloucks commented Mar 20, 2020 •

edited

Loading

aloucks commented Mar 24, 2020 •

edited

Loading

aloucks commented Mar 25, 2020 •

edited

Loading

aloucks commented Mar 25, 2020 •

edited

Loading

aloucks commented Mar 25, 2020 •

edited

Loading

aloucks Mar 26, 2020 •

edited

Loading