[teoxoy]

Connections

• Firefox bug 2084148

Firefox's wgpu update (Bug 2066780) needs this since queue_submit only returns the submission index now and we have no way to tell if the submission succeeded (we can't use an error scope since validation errors should still be surfaced to users).

Firefox was also incorrectly using the presence of Result::Err to mean the submission failed but that's not correct since the inner call to maintain (below the call to HAL submit) could error. The error would be propagated but the submission did reach the driver and our lifetime tracking. This PR also enables us to fix this issue.

Description
If submissions were not successful, a subsequent call to poll with the returned SubmissionIndex would have raised a validation error anyway.

Making the result type Option<SubmissionIndex> hints to users that the submission has not reached the driver so fences/events/semaphores that were attached to HAL queues will never be signalled.

Testing
Existing tests.

Squash or Rebase?
Squash

Checklist

• I self-reviewed and fully understand this PR.
• CHANGELOG.md entries for the user-facing effects of this change are present.
• The PR is minimal, and doesn't make sense to land as multiple PRs.
• Commits are logically scoped and individually reviewable.
• The PR description has enough context to understand the motivation and solution implemented.
• (If applicable) WebGPU implementations built with wgpu may be affected behaviorally.
• (If applicable) Validation and feature gates are in place to confine behavioral changes.
• (If applicable) Tests demonstrate the validation and altered logic works.

[andyleiserson]

Suggested change

	Some(0)
	// This is a (rather poor) sentinel that means "submission result unknown".
	// On the WebGPU backend, status is only available via validation errors.
	Some(0)

[teoxoy]

Maybe we should change this to None since it's a submission with an unknown index so it can't really be waited on via a poll?

[andyleiserson]

I was thinking that it's more important to distinguish success from failure than "have wait cursor" from "don't have wait cursor". Or I also considered defining a 3-way enum, or trying to offer an API where the app can opt in to "I want this to work on the WebGPU backend" validation, which would cause operations that won't work on the WebGPU backend to fail even when running against core.

[teoxoy]

I started thinking of SubmissionIndex as a "wait cursor" because that's how it's used in the wgpu API; it's opaque and only poll with PollType::Wait can receive it. I'm also thinking to avoid saying the submission was successful since it might be confused with "submit didn't error" but it can due to the inner call to maintain.

poll seems useless on the WebGPU backend since there is no way to synchronously wait on a submission in WASM (when specifying PollType::Wait) and PollType::Poll is a no-op since the browser is already making progress via its event loop.

To normalize behavior across backends I think we need a new poll error variant (PollError::CantWait) that's always returned on WebGPU if PollType::Wait is supplied and have submit always return None.

cc @cwfitzgerald

[cwfitzgerald]

The design of poll and the submission ID was trying to be clever to make portable code easier - its contract that calling poll will wait until the callbacks associated with that submission with eventually resolve. Which means a no-op on wasm is a valid implementation.

Now if that was the correct solution or too clever? I'm not sure. I could see the utility of switching SubmissionIndex from being opaque to being transparent and then using an enum to determine what happened inside a poll.

[teoxoy]

its contract that calling poll will wait until the callbacks associated with that submission with eventually resolve. Which means a no-op on wasm is a valid implementation.

That seems to be its contract if its argument is PollType::Poll which I think is fine but if it's PollType::Wait then it must wait either on one specific submission or the last active submission which doesn't happen on WASM. I think this is a portability issue since it's observable: you can race poll(PollType::Wait) with on_submitted_work_done or map_async; timing the operations would also reveal that poll(PollType::Wait) completes immediately.

[teoxoy]

I'm inclined to have the WebGPU backend return None to make this portable. Can I do it in this PR so I can also update the docs above? @cwfitzgerald @andyleiserson

The PR should be good to land otherwise.

[andyleiserson]

I'm not sure I follow the proposal. Some(SubmissionIndex) vs. None, where the former means waitable, the latter means "not waitable"? WebGPU always returns "not waitable" (even if the submission was successful), but that's okay, because the place we want to consume this is Firefox, which doesn't use the WebGPU backend, and can assume waitable == success?

I guess that is okay. I would probably belabor the point a bit more in the docs:

Returns Some if the submission started and can be waited on via a poll, or None otherwise. On the WebGPU backend, always returns None (even for successful submissions), because the WebGPU backend does not support waiting for a particular submission.

"Until the callbacks associated with that submission with eventually resolve" seems too clever to me. Is it documented somewhere? The docs on Device::maintain say "The poll_type argument tells if this function should wait for a particular submission index to complete, or if it should just poll the current status.", which doesn't seem right to me for the "eventually resolve" contract, but I guess this is wgpu-core and it's not wrong as long as the scope is understood correctly (very easy to misunderstand IMO). In the "eventually resolve" regime, poll/maintain doesn't really do anything (from the application's perspective), right? You just have to call it or else your callbacks are not guaranteed to fire. Maybe PollType::Wait should return an unsupported error on the WebGPU backend? (Unfortunately that seems likely to turn up a lot of false positives in apps in addition to real problems.)

But in any case None seems like a better return than Some(0) for the non-waitable case. Portable code can still just ignore the result from submit and only use PollType::Poll (which is the only one that works portably, anyways). It doesn't seem useful to maintain the usage sequence "Return a fake SubmissionIndex of zero which the app can then pass to PollType::Wait, which will ignore it (on WebGPU)."

[teoxoy]

I made the changes in this PR. 668b12f
The commits will get squashed anyway.

[andyleiserson]

Maybe worth adding the "on wgpu-core-based backends" caveat here as well.

[andyleiserson]

Not directly related to this change, but related more generally to the question of handling submit errors, Vulkan says:

For any command that may return VK_ERROR_DEVICE_LOST, for the purpose of determining whether a command buffer is in the pending state, or whether resources are considered in-use by the device, a return value of VK_ERROR_DEVICE_LOST is equivalent to VK_SUCCESS.

I think this means we can't drop resources immediately after a submit that fails with VK_ERROR_DEVICE_LOST, that we need to wait for idle first. Filed #10222 for this.

[sagudev]

Firefox's wgpu update (Bug 2066780) needs this since queue_submit only returns the submission index now and we have no way to tell if the submission succeeded (we can't use an error scope since validation errors should still be surfaced to users).

HACK: push additional error scope then pop it, do whatever FF is doing with error then return it back into the error sink using device_handle_error

[teoxoy]

That's a good suggestion - I ended up doing that to unblock the wgpu update. It's nice that the maintain call will also not interfere since it doesn't generate validation errors - it can only lose the device.

[ErichDonGubler]

Made Firefox bug 2084148 to track this.

[teoxoy]

@Mergifyio queue