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

feature: Timestamp queries #3636

Merged
merged 56 commits into from
Aug 2, 2023

Conversation

FL33TW00D
Copy link
Contributor

@FL33TW00D FL33TW00D commented Apr 2, 2023

Checklist

  • Run cargo clippy.
  • Run RUSTFLAGS=--cfg=web_sys_unstable_apis cargo clippy --target wasm32-unknown-unknown if applicable.
  • Add change to CHANGELOG.md. See simple instructions inside file.

Connections
https://github.com/gfx-rs/wgpu/pull/2528/files
gpuweb/gpuweb#1325
gpuweb/gpuweb#2046
gfx-rs/metal-rs#263

Description
This is a WIP implementation of timestamp queries for compute/render passes. Only implemented for Metal and compute for now.

TODO

  • Release required changes on metal-rs here.
  • Implement and test on Metal for render passes.
  • Implement and test for all other back ends for compute and render passes.
  • Resolve issue with attaching the same sample buffer multiple times.
  • Update all existing examples using ComputePassDescriptor and RenderPassDescriptor.

Testing
Modified hello-compute and hello-triangle and successfully recorded the execution time. Only tested on M1 so far.

@FL33TW00D FL33TW00D marked this pull request as draft April 2, 2023 14:41
@FL33TW00D FL33TW00D changed the title feature: Timestamp queries on Metal feature: Timestamp queries Apr 2, 2023
Cargo.toml Outdated Show resolved Hide resolved
@cwfitzgerald
Copy link
Member

For the other backends, can't we just implement in terms of other existing operations on the command encoder?

Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

super cool PR, looking forward to this. Maybe tomorrow I get finally to picking up the flag there if @FL33TW00D doesn't have more in the pipe already?

Haven't gotten to actually looking at the actual Metal code yet :(.
Sadly the spec moved already on a bit, so things are supposed to look a lil bit different now, put comments at the key place so it's easy to find.

otherwise also on the todo list belong

  • update comments on TIMESTAMP_QUERY feature
  • implement (i.e. forward things in) web.rs version

wgpu/src/lib.rs Outdated Show resolved Hide resolved
wgpu/src/lib.rs Outdated Show resolved Hide resolved
wgpu/src/lib.rs Outdated Show resolved Hide resolved
wgpu/src/lib.rs Outdated Show resolved Hide resolved
wgpu/examples/hello-triangle/main.rs Outdated Show resolved Hide resolved
wgpu/examples/hello-compute/main.rs Outdated Show resolved Hide resolved
wgpu/Cargo.toml Outdated Show resolved Hide resolved
wgpu-core/src/command/render.rs Outdated Show resolved Hide resolved
wgpu-core/src/command/compute.rs Outdated Show resolved Hide resolved
@FL33TW00D
Copy link
Contributor Author

FL33TW00D commented Apr 30, 2023

@Wumpf Really appreciate you looking through this, I left it as is due to the release of Chrome 113 being a higher priority.

The metal PR is here: gfx-rs/metal-rs#265

There is still a problem with the counter sample buffer not being kept alive long enough.

@Wumpf
Copy link
Member

Wumpf commented May 1, 2023

@FL33TW00D I copied your branch over to this fork and continued from there - did the move over the (simpler!) WebGPU spec update so far.
If you give me write access to yours I can push there and we can continue here or we create a new PR to commence work there, up to you :)
https://github.com/rerun-io/wgpu/tree/feature/metal-timestamp

@Wumpf Wumpf force-pushed the feature/metal-timestamp branch from 78e7896 to 9680a18 Compare May 7, 2023 08:18
@Wumpf
Copy link
Member

Wumpf commented Jul 23, 2023

got everything up to date and address review comments but there is some work left to do after adding the tests:

  • Mac: @ MoltenVK hard-crashes when it should fail with a known failure. Might be an issue in MoltenVK, need to investigate
  • Windows: Turns out that all queries had an undefined behavior issue that is caught by the debug layer when you tried to resolve a queryheap slot to which nothing has been written. WebGPU spec sounds like this should just pass. Which means that we need to track all uninitialized heap spots. Other APIs don't seem to have any issue there, but it's easiest to solve this on the wgpu-core level (and on the flip side adds the kind of overhead to wgpu-hal that we usually don't want to have there)
  • Linux: Ci gets stuck on the new test, unclear why

hoping to get to these at some point this week

@Wumpf
Copy link
Member

Wumpf commented Jul 30, 2023

the whole "don't copy uninitialized query slots" is a huge rabbit hole. After trying a couple of things I decided do adjourn this, for details see #3993

@Wumpf
Copy link
Member

Wumpf commented Jul 30, 2023

All green now again!
@cwfitzgerald I know you advocated for putting this in after arcanization, but it's been quite a while and there's a bunch of things that want to be built on top of this PR:

  • Solving uninitialized queries
  • support inter-pass queries on Metal
  • support inter-encoder queries on Metal
  • web-backend implementation
  • Demo part (@crowlKats )

@Wumpf Wumpf requested a review from cwfitzgerald July 30, 2023 14:59
Copy link
Member

@cwfitzgerald cwfitzgerald 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 positively amazing work. Short of a couple nits, this is good to land.

examples/timestamp-queries/src/main.rs Outdated Show resolved Hide resolved
examples/timestamp-queries/src/main.rs Outdated Show resolved Hide resolved
examples/timestamp-queries/src/main.rs Outdated Show resolved Hide resolved
wgpu-core/src/command/compute.rs Outdated Show resolved Hide resolved
@cwfitzgerald
Copy link
Member

CI is fail due to taiki-e/install-action#178

@cwfitzgerald cwfitzgerald merged commit 3305e88 into gfx-rs:trunk Aug 2, 2023
17 of 20 checks passed
@Wumpf Wumpf mentioned this pull request Aug 6, 2023
3 tasks
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.

6 participants