Skip to content

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

Merged
merged 1 commit into from
Jan 16, 2021

Conversation

cwfitzgerald
Copy link
Member

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.

@cwfitzgerald cwfitzgerald requested a review from kvark January 5, 2021 01:50
Copy link
Contributor

@monocodus monocodus bot left a 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.

@cwfitzgerald
Copy link
Member Author

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.

@cwfitzgerald
Copy link
Member Author

Traces are added

@cwfitzgerald cwfitzgerald force-pushed the queries branch 3 times, most recently from f16db06 to bbdd653 Compare January 7, 2021 18:56
Copy link
Member

@kvark kvark left a 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!

query_type: SimplifiedQueryType,
query_set_id: id::QuerySetId,
query_index: u32,
reset_state: &mut Option<&mut QueryResetMap>,
Copy link
Member

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>?

Copy link
Member Author

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.

@cwfitzgerald
Copy link
Member Author

Finishing up wgpu-rs PR then I'll undraft, ready for round 3 review.

@cwfitzgerald
Copy link
Member Author

Everything seems to work fine, so we're good to go :)

Copy link
Member

@kvark kvark left a 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.

@cwfitzgerald
Copy link
Member Author

Addressed; Thanks for the detailed reviews :)

bors r=kvark

@bors
Copy link
Contributor

bors bot commented Jan 16, 2021

@bors bors bot merged commit 9dce0af into gfx-rs:master Jan 16, 2021
bors bot added a commit to gfx-rs/wgpu-rs that referenced this pull request Jan 16, 2021
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>
kvark pushed a commit to kvark/wgpu that referenced this pull request Jun 3, 2021
715: Add pipeline statistics and timeline queries r=kvark a=cwfitzgerald

Blocked on gfx-rs#1128.

Co-authored-by: Connor Fitzgerald <connorwadefitzgerald@gmail.com>
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

Successfully merging this pull request may close these issues.

WebGPU "Queries" implementation (for profiling purposes)
2 participants