-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add pipeline statistics and timeline queries #1128
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
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.
Checker summary (by rust_clippy):
The tool has found 2 warnings, 2 errors.
Alright, this is still blocked on gfx updates and I need to add queries to traces, but the core changes are ready for second pass review. |
Traces are added |
f16db06
to
bbdd653
Compare
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.
Please read all the comments before starting to address them :)
Thank you for your patience!
wgpu-core/src/command/query.rs
Outdated
query_type: SimplifiedQueryType, | ||
query_set_id: id::QuerySetId, | ||
query_index: u32, | ||
reset_state: &mut Option<&mut QueryResetMap>, |
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.
oh wow, first time seeing this pattern!
Why is it not just reset_state: Option<&mut QueryResetMap>
?
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.
Because I need to use the option later on as well. If I pass it by value I can't use it later in the function. I need to get (from the option) a &mut &mut QueryResetMap
, which is only possible when the reference to the option is also mut.
Finishing up wgpu-rs PR then I'll undraft, ready for round 3 review. |
Everything seems to work fine, so we're good to go :) |
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.
Amazing work, the code looks great!
Just a few small nits, one of them (about the query size check) needs to be addressed, others up to your consideration.
Addressed; Thanks for the detailed reviews :) bors r=kvark |
715: Add pipeline statistics and timeline queries r=kvark a=cwfitzgerald Blocked on gfx-rs/wgpu#1128. Co-authored-by: Connor Fitzgerald <connorwadefitzgerald@gmail.com>
715: Add pipeline statistics and timeline queries r=kvark a=cwfitzgerald Blocked on gfx-rs#1128. Co-authored-by: Connor Fitzgerald <connorwadefitzgerald@gmail.com>
Connections
Closes #721
Description
This adds Pipeline Statistics queries and Timeline queries. Shoutout to @z2oh for doing a good chunk of the work to enable this feature.
Currently blocked on both gfx-rs/gfx#3563 and rollup of gfx-hal mutability changes, but is ready to review.
Testing
Tested in various ways on the hello-compute and hello-triangle example, tested validation extensively. Queries will be permanently added to the mipmap example once the wgpu-rs PR is finished.