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

Separate out ComputeCommand id->arc resolve (a step towards no lifetimes on wgpu::ComputePass) #5432

Merged
merged 15 commits into from
Apr 23, 2024

Conversation

Wumpf
Copy link
Member

@Wumpf Wumpf commented Mar 24, 2024

Connections

Part of a series towards lifetime removal:

Description


Background

This is working towards removing the dreaded lifetimes on wgpu::ComputePass & wgpu::RenderPass. I'm starting with compute passes since they have a much smaller API surface area.

Today, the lifetime on the wgpu objects protect us from dropping resources that may be used during pass execution. For example if one passes a pipeline to a render/compute pass, it musn't be dropped until the pass is executed/ended since the pass currently only remembers the pipeline's id.
To allow dropping it (and thus removing the lifetime constraint), we have to bump the reference count of all resources going into a pass upon recording and not (as done today) upon finishing the pass. Ever since Arcanization landed this is technically possible with manageable overhead.

This PR

This PR introduces ArcComputeCommand which is to ComputeCommand like the existing ArcRenderCommand to RenderCommand (ArcRenderCommand is only used in RenderBundle today, but this likely going to change soon as well!).

The goal in a follow-up PR is for ComputePass to record ArcComputeCommand directly, reserving ComputeCommand solely for tracing.
Instead, in this iteration recording operations on ComputePass still push ComputeCommand into an internal vector which then is converted just before finish to ArcComputeCommand, thus separating out id->Arc resolve of the actual execution. -> Only recording replay will need to do this bulk conversion in the future, but it's not going away until ids are gone. The pass execution itself now relies already on ArcComputeCommand.

Alternatives considered

The original idea was to extend ComputeCommand to hold both ids as well as optional Arc in order to avoid duplication of the command enum. However what made me decide differently:

  • This would have required making ComputeCommand generic over a HalApi making the trace infrastructure infected with this. It looked to me like some type erasure would have been needed
  • There's precedence with ArcRenderCommand
  • While the redundancy in the type isn't pretty, keeping recording separate from non-recording is quite advantageous in many ways: the actually used command enum doesn't get any bulkier for the non-trace case and the trace-only datastructures can be separated out and stay under the recording feature (-> ArcComputeCommand can become just ComputeCommand and what's ComputeCommand today becomes TraceComputeCommand!)

Testing
Regular unit tests run compute, this should cover this well enough.

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.

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.

Looking good, have some comments

wgpu-core/src/command/compute.rs Outdated Show resolved Hide resolved
wgpu-core/src/command/compute_command.rs Outdated Show resolved Hide resolved
wgpu-core/src/track/metadata.rs Outdated Show resolved Hide resolved
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.

Nice!

wgpu-core/src/command/compute.rs Outdated Show resolved Hide resolved
@Wumpf Wumpf enabled auto-merge (squash) April 23, 2024 06:52
@Wumpf Wumpf merged commit edf1a86 into gfx-rs:trunk Apr 23, 2024
25 checks passed
@Wumpf Wumpf deleted the separate-computepass-arc-resolve branch April 23, 2024 07:04
@Wumpf Wumpf mentioned this pull request May 5, 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.

None yet

2 participants