-
Notifications
You must be signed in to change notification settings - Fork 938
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
Tracing Infrastructure #619
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an autogenerated code review, no new suggestions, fix old one
The .monocodus
config not found in your repo. Default config is used.
Check config documentation here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an autogenerated code review, new suggestions: 3
The .monocodus
config not found in your repo. Default config is used.
Check config documentation here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an autogenerated code review, new suggestions: 13
The .monocodus
config not found in your repo. Default config is used.
Check config documentation here
Have you considered using |
@aloucks did you read to the end? This is exactly the last item of "Alternatives considered" section, at the bottom of the description. |
@kvark Ah, I see that now 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great overall! Thanks for doing this, I think it will greatly help debugging. There is a bit of complexity added by the #[cfg(feature = "trace")]
in the middle of function bodies, generics over label, map_label
, etc. but it seems worthwhile here.
@@ -4,13 +4,16 @@ | |||
|
|||
#[cfg(feature = "peek-poke")] | |||
use peek_poke::PeekPoke; | |||
#[cfg(feature = "serde")] | |||
use serde::{Deserialize, Serialize}; | |||
#[cfg(feature = "replay")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be confusing for people looking to enable serialization to have to enable trace
or replay
. Most people would probably look for a serde
or serde1
feature because that's the common place to look.
For example, users may just want to serialize their types, but shouldn't necessarily need to understand the association between serialization and trace/replay (even if trace/replay happen to also require serialization).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I agree. Also, I think there is value to separating those even outside of the trace/replay scope: quite often the user knows exactly which side is needed, and deriving both affects compile times quite a bit.
Perhaps, this is something to address by documentation?
ptr, | ||
}; | ||
|
||
macro_rules! gfx_select { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we expose gfx_select
from wgpu-core and use that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
possibly, yes. Today, the one used in wgpu-rs is slightly different though. We may end up with small differences across the users, and the macro itself is pretty small. Let's keep an eye on it for follow-ups?
bors r=grovesNL |
17: Update for wgpu-core r=kvark a=kvark Depends on gfx-rs/wgpu#619 Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com>
17: Update for wgpu-core r=kvark a=kvark Depends on gfx-rs/wgpu#619 Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com>
291: Update for wgpu-core r=grovesNL a=kvark Depends on gfx-rs/wgpu#619 Closes #285 Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com>
291: Update for wgpu-core r=grovesNL a=kvark Depends on gfx-rs#619 Closes gfx-rs#285 Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com>
Closes #289
Downstream changes: gfx-rs/wgpu-rs#291 gfx-rs/wgpu-native#17
Note: close to half of the new LOCs belong to
Cargo.lock
, which we are now including since we have a binary target crateplayer
.Why do we need that?
Have you ever seen a case that doesn't work for somebody? Often, reproducing the complete setup requires elaborate toolchains, external resources, and closed-source code. Even with all of that, the difference between platforms may hide the issue. RenderDoc or Metal captures do not help, since they are extremely driver/hardware-dependent.
This problem is amplified for WebGPU implementation in Gecko. Some workloads are rendered incorrectly, and debugging them within the browser becomes too much of a challenge.
Temporary limitations (can be fixed later):
s/Vulkan/Metal/g
)Tracing allows us to swiftly isolate and reproduce rendering issues. We can now consider a Warden-like testing and benchmarking framework built around the traces.
What the hell it is?
This is a state of art tracing infrastructure built into
wgpu-core
. It traces all the commands and resource changes, very similarly toapitrace
. Tracing code is enabled by a "trace" feature, and the actual support is enabled at the device instantiation, controlled at run-time. There is no run-time overhead for running with tracing feature but in disabled state.The result of tracing is a directory, containing the following files:
The new "player" application can be used to replay the traces. It's launched as:
When built with "winit" feature, it's able to replay the workloads that operate on a swapchain. It renders each frame consequently, then waits for the user to close the window. When built without "winit", it launches in console mode and can replay any trace that doesn't use swapchains.
How does it look?
Having the trace in RON makes it easy to inspect visually (by humans!). There is no separate "dump" phase. Each submission is labelled, and the trace is easy to navigate. Moreover, everything is human-editable: you can tweak it to see what changes would be needed in an application in order to fix a rendering issue. For example, you can change the blend modes and re-play.
Alternatives considered
Q: Why not just add support to
apitrace
(orrenderdoc
) instead?A: There is a benefit of having this Rusty: we can use heavy lifters like Serde and RON, we use pattern matching extensively. We can still consider something more independent of the language in the future, something with C API that would work for both Dawn and wgpu-native. I just don't want to invest in that now, and going for the local solution was quite straightforward.
Q: Why not slice the execution at a particular time, instead of recording everything from the beginning?
A: I think both approaches are needed, but for different purposes. Tracing allows to share and reproduce rendering issues. Also can aid in the test corpus building. Slicing vertically would help to investigate internal state problems. We can still get that in the future.
Q: Why RON and not JSON/YAML/etc?
A: Experience with WebRender proved RON to be ideal for this kind of use. Technically, there are no blockers from writing to another format - it's a matter of a single line change, plus some extra dependencies. After all, this is all going through Serde :)