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

Remove lifetime dependency of ComputePass to its parent command encoder #5620

Merged
merged 5 commits into from
May 29, 2024

Conversation

Wumpf
Copy link
Member

@Wumpf Wumpf commented Apr 28, 2024

Connections

Part of a series towards lifetime removal on compute passes:

Description

ComputePass had a static lifetime dependency to its parent encoder. This PR removes this (ComputePass<'a> -> ComputePass) by having ComputePass own an Arc to its parent encoder. A side-effect of this is that ComputePass creation can now fail in theory (if an invalid id is passed or commandbuffer allocation fails for some reason).

Another wide reaching side effect is that command encoders can now be in a Locked state. (via WebGPU spec) any operation on a locked command encoder fails and makes the encoder invalid.

The last remaining lifetime dependency to resolve on ComputePass after this PR is in ComputePassTimestampWrites<'a>: We currently don't bump the reference count on the referenced QuerySet. This is in fact more of a pre-existing bug since destroying a QuerySet after recording it in a ComputePass and before closing the pass already causes issues!

Testing

  • added a test that verifies that a command encoder can be dropped child compute pass is still live
  • added a that that while a compute pass is being recorded, trying to do any operation on the encoder fails and makes it invalid

Checklist

  • Run cargo fmt.
  • Run cargo clippy. If applicable, add:
    • --target wasm32-unknown-unknown
    • --target wasm32-unknown-emscripten
  • Run cargo xtask test to run tests.
  • Add change to CHANGELOG.md. See simple instructions inside file.

@Wumpf Wumpf force-pushed the lifetime-free-computepass2 branch from ed88c1f to 70c925d Compare May 5, 2024 10:02
@Wumpf Wumpf mentioned this pull request May 5, 2024
6 tasks
@Wumpf Wumpf force-pushed the lifetime-free-computepass2 branch from 70c925d to fa15a9c Compare May 26, 2024 10:31
@Wumpf Wumpf marked this pull request as ready for review May 26, 2024 10:31
@Wumpf Wumpf requested a review from a team as a code owner May 26, 2024 10:31
@Wumpf Wumpf force-pushed the lifetime-free-computepass2 branch from fa15a9c to 3694e1e Compare May 26, 2024 11:08
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.

Most of this makes sense, have a small comment here and there.

Test looks great, thanks for including it!

wgpu-core/src/command/mod.rs Show resolved Hide resolved
@Wumpf Wumpf enabled auto-merge (squash) May 29, 2024 22:23
@Wumpf Wumpf merged commit 5889501 into gfx-rs:trunk May 29, 2024
25 checks passed
@Wumpf Wumpf deleted the lifetime-free-computepass2 branch May 29, 2024 22:43
@Wumpf Wumpf mentioned this pull request Jun 26, 2024
6 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.

2 participants