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

Add timestamp query support to compute boids #323

Merged
merged 6 commits into from
Nov 10, 2023

Conversation

beaufortfrancois
Copy link
Collaborator

This PR adds timestamp query support to the compute boids sample by showing render pass and compute pass durations.

I've tried several approaches before this attempt:

  • Creating a queryset each frame but it fails very quickly on macOS with DAWN_OUT_OF_MEMORY_ERROR.
  • Waiting for resultBuffer.mapAsync to settle in frame() but this makes UI janky as expected.

image

@beaufortfrancois
Copy link
Collaborator Author

@kainino0x Can you review?

@greggman
Copy link
Collaborator

greggman commented Nov 9, 2023

I had no issue with this design:

https://jsgist.org/?src=8b8ad1e7e7cfd92b6275927b9d61de23

Which is effectively, you have an array of free queries and result buffers, when you start a frame you ask for a query/result buffer. If there are none free you allocate new ones. You then mapAsync.then() to get the result. After reading the result you add the query and result buffer back to the free list. You end up at most with 2-4 of these after a few frames and then no more allocations

You might get the result later 1-3 frames later but you get no jank

@beaufortfrancois
Copy link
Collaborator Author

Thanks for sharing @greggman! Is the pattern you're recommending is the one we should promote or is it one among others? I'm genuinely asking as it's not clear to me ;)

@beaufortfrancois
Copy link
Collaborator Author

I've removed resultBuffer.mapState == 'unmapped' check in latest patch by making sure buffers are created each frame, which work great locally.

I humbly think having a list of free buffers is a bit too much for a sample.

@greggman
Copy link
Collaborator

greggman commented Nov 9, 2023

Is the pattern you're recommending is the one we should promote or is it one among others? I'm genuinely asking as it's not clear to me ;)

I don't know either, haha. I'd like to know too.

I think having a list of buffers is trivial code.

freeBuffers = [];

buffer = freeBuffers.pop() || device.createBuffer(...);

...

buffer.mapAsync().then(_ => { 
  ...
  freeBuffers.push(buffer);
});

src/sample/computeBoids/main.ts Outdated Show resolved Hide resolved
src/sample/computeBoids/main.ts Outdated Show resolved Hide resolved
src/sample/computeBoids/main.ts Outdated Show resolved Hide resolved
@beaufortfrancois
Copy link
Collaborator Author

I've addressed @kainino0x and @greggman feedback in latest patch.

Copy link
Collaborator

@kainino0x kainino0x left a comment

Choose a reason for hiding this comment

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

LGTM but had a few nits which I resolved in the commit I'll push now:

  • Number is hard to read because it flickers too fast, and it's too big (I changed it to µs)
  • Some comments would help explain to readers

@kainino0x
Copy link
Collaborator

image

@kainino0x kainino0x merged commit f7f813c into webgpu:main Nov 10, 2023
1 check passed
@beaufortfrancois beaufortfrancois deleted the timestamp-query branch November 11, 2023 05:36
@beaufortfrancois
Copy link
Collaborator Author

Thank you very much @kainino0x! It looks great

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.

3 participants