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.

Sign up for GitHub

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

Jump to bottom

Should command buffers be invalidated by an invalid submit? #4599

Closed
kainino0x opened this issue Apr 24, 2024 · 5 comments · Fixed by #4608
Closed

Should command buffers be invalidated by an invalid submit? #4599

kainino0x opened this issue Apr 24, 2024 · 5 comments · Fixed by #4608
Labels
api resolved Resolved - waiting for a change to the API specification api WebGPU API
Milestone
Milestone 1

Comments

@kainino0x
Copy link
Contributor

kainino0x commented Apr 24, 2024 •
edited

Tangentially related to #4593 (just because that added an extra rule to submit).

Say you have two command buffers cb_valid and cb_invalid:

queue.submit([cb_valid, cb_invalid]); // Invalid. cb_valid does not execute.
queue.submit([cb_valid]); // Is cb_valid still valid at this point? Spec says yes.

Currently, the spec says it is still valid. It so happens that Dawn incorrectly implements this and makes cb invalid even if the submit doesn't happen (so there's no breaking change potential for anything that currently works on Chrome). It probably isn't actually useful for it to be valid, so being more conservative might be good.
@kainino0x kainino0x added this to the Milestone 1 milestone Apr 24, 2024
@kainino0x kainino0x added the needs-cts-issue This change requires tests (or would need tests if accepted), but may not have a CTS issue filed yet label Apr 24, 2024
@kainino0x kainino0x mentioned this issue Apr 24, 2024
Merged
8 tasks
@jimblandy
Copy link
Contributor

jimblandy commented Apr 25, 2024 •
edited

wgpu would prefer for an invalid submit to invalidate all the command buffers, as well.

@toji
Copy link
Member

toji commented Apr 26, 2024

Worth noting that I've searched through the CTS and haven't found any tests that exercise the command buffer invalidation behavior either way. We don't even appear to test that command buffers are invalidated after a successful submit, so that's a clear gap in our testing right now.

This was referenced Apr 26, 2024
Merged
Merged
@kainino0x kainino0x added api WebGPU API and removed for webgpu editors meeting labels Apr 29, 2024
@mwyrzykowski
Copy link

I would prefer this as well, to avoid running out of command buffers as noted in #4622

WebKit currently says cb_valid is still valid. However, if the application doesn't resubmit cb_valid, then they run the risk of running out of available command buffers.

@litherum
Copy link
Contributor

litherum commented May 15, 2024 •
edited

Be aware that, if the group does this, authors will probably be incentivized to do the anti-pattern of

queue.submit([cb_valid]);
queue.submit([cb_invalid]);

instead of

queue.submit([cb_valid, cb_invalid]);

@kainino0x kainino0x added the api resolved Resolved - waiting for a change to the API specification label May 15, 2024
@kainino0x kainino0x removed the needs-cts-issue This change requires tests (or would need tests if accepted), but may not have a CTS issue filed yet label May 15, 2024
@mwyrzykowski mwyrzykowski mentioned this issue May 20, 2024
Closed
@Kangz
Copy link
Contributor

Kangz commented May 21, 2024

GPU Web WG 2024-05-15
  • BJ: This is something that came up as we were doing our spec revisions. Looking through the logic for the spec versus Chrome, Chrome would invalidate every buffer you submit, but the spec says that if one command buffer is invalid, the remainder of submitted buffers remain valid. There seems to be general agreement that once you’ve submitted a list of command buffers, whether the submit succeeds or not, all the command buffers should be invalidated. points out that the specified behavior means you may have a bunch of valid command buffers lying around that you weren’t expecting.
  • BJ: Myles points out that this might incentivize people to make lots of little commits, which is an antipattern. A more effective remedy for them would be to check the command buffers’ validity ahead of time by popping the error scope.
  • KG: Is there a general consensus?
  • CW: Seems like it.
  • KG: If we wanted to solve the problem that Myles points out, we could specify that they all get maximally consumed: if there’s a submission failure in the first command buffer, it keeps trying to consume them until it’s done with the list.
  • BJ: I think that’s what’s being proposed. If any of them are invalid, then all are invalid
  • CW: Paraphrasing kelsey: command buffers are checked for duplication
  • KG: It’s a two-layer thing. The main principle is that this is an operation that consumes something. If it were memory management, it would be essential to make this clear. Fundamentally, this kind of operation should consume the buffer and take it back as much as possible, even if it fails, because it leaves the caller in a difficult state to interpret. Past that, we need to consider how to avoid discouraging less-than-ideal behavior.
  • MM: I’m not trying to stop the train. In the code blocks in the issue, in the first code block the execution is different between the two examples. Command buffers are generally not going to be independent. So I think we should just continue with this.
  • CONSENSUS: All command buffers passed to any submit are invalidated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
api resolved Resolved - waiting for a change to the API specification api WebGPU API
Projects
None yet
Milestone
Milestone 1
Development

Successfully merging a pull request may close this issue.

Invalidate all command buffers passed to submit, whether the submit is valid or not. gpuweb/gpuweb
6 participants
@Kangz @kainino0x @jimblandy @toji @litherum @mwyrzykowski