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

Bind group layout dedup #3925

Merged
merged 4 commits into from
Aug 25, 2023
Merged

Bind group layout dedup #3925

merged 4 commits into from
Aug 25, 2023

Conversation

nical
Copy link
Contributor

@nical nical commented Jul 12, 2023

Checklist

  • Run cargo clippy.
  • Add change to CHANGELOG.md. See simple instructions inside file.

Description

Currently wgpu-core implement bind group layout deduplication only when it creates its own resource IDs. In other words it works for wgpu but not in Firefox.

This PR bridges the gap by allowing an optional indirection in bind group layouts: each BGL may store an ID referring to its "deduplicated" BGL. The indirection ID is never set in wgpu (when the IDs are generated by wgpu core), so in this configuration the behavior is unchanged (the indirection ID is always None).

When referring to a BGL the rest of the code must make sure to follow the indirection. The exception is command buffer processing which is considered hot code and where we first validate against the provided BGL ID and only follow the indirection if the initial check failed.

The main pain point with this approach is the various places where wgpu-core manually updates reference counts: we have to be careful about following the indirection to track the right BGL. I suspect that arcanization makes this better but I haven't tried to rebase this on top of it yet.

On the plus side, the plumbing to enable the validation to follow the indirection will make it very easy to decorate validation errors with labels in a followup PR.

This is the main thing preventing bevy apps from running in Firefox.

Alternatives

Deduplication could be re-implemented in Firefox's DOM glue, however it has a few disadvantages:

  • The deduplication logic would be duplicated.
  • More importantly, Firefox's WebGPU DOM/IPC glue is currently very simple. It is mostly a bunch of proxies for the various objects created and manipulated by wgpu-core. If BGL deduplication was implemented there, these proxies would have to reimplement much of the lifetime tracking logic that is present in wgpu-core: bind groups and pipeline layouts must keep their layouts alive, pipelines must keep their pipeline layouts alive, etc. We would prefer to preserve the simplicity of these proxies.

Can we do better in the long run?

I hope so. In a potential future where wgpu-core generates its own IDs even when run in Firefox, everything could be simpler and most of what's in this PR would be unnecessary. We have been discussing this with @jimblandy our plans aren't very well formed and it is rather far down the priority list but I'll try to summarize that in another issue.

Testing

I added a test which actually only covers wgpu's default configuration and not the new code. It's a bit tedious to test that (Need to write the test against wgpu-core, add an identity factory, etc), however that code path is covered by the WebGPU CTS so regressions will be caught in Firefox's CI (with a delay unfortunately).

@nical nical force-pushed the bgl-dedup branch 2 times, most recently from 1af673a to 47ca667 Compare July 12, 2023 12:45
@nical nical changed the title Bgl dedup Bind group layout dedup Jul 13, 2023
@kdashg
Copy link

kdashg commented Aug 14, 2023

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.

Code looks fine minus nit

wgpu-core/src/device/global.rs Outdated Show resolved Hide resolved
@nical nical force-pushed the bgl-dedup branch 3 times, most recently from 02c8867 to d50d9e1 Compare August 17, 2023 15:34
wgpu-core/src/device/global.rs Outdated Show resolved Hide resolved
wgpu-core/src/device/resource.rs Outdated Show resolved Hide resolved
wgpu-core/src/device/life.rs Show resolved Hide resolved
The code is specific to bind group layout ids and the generic parameter gets in the way.
The test actually covers wgpu's configuration where deduplication does not require the indirection rather than the new code. I used it to debug the new code with the configuration hard-coded. It's tedious to add a test to cover dedpuplication of bind group layouts for users of wgpu_core to provide their IDs, we can rely on the CTS which has test for that.
Currently wgpu-core implement bind group layout deduplication only when it creates its own resource IDs. In other words it works for wgpu but not in Firefox.
This PR bridges the gap by allowing an optional indirection in bind group layouts: each BGL may store an ID referring to its "deduplicated" BGL.
When referring to a BGL the rest of the code must make sure to follow the indirection. The exception is command buffer processing which is considered hot code and where we first validate against the provided BGL ID and only follow the indirection if the initial check failed.

The main pain point with this approach is the various places where wgpu-core manually updates reference counts: we have to be careful about following the indirection to track the right BGL.
@teoxoy teoxoy dismissed cwfitzgerald’s stale review August 25, 2023 19:48

Has been addressed

@nical nical merged commit 399637e into gfx-rs:trunk Aug 25, 2023
20 checks passed
@gents83
Copy link
Contributor

gents83 commented Aug 26, 2023

@nical correct me if I'm wrong - what you're trying to do here is to get a BGL that already exist if descriptor is the same or if you already have the id ?
It's not compatible with Arcanization so I'll have to reimplement it and I would like to be sure to understand it 👍

@nical
Copy link
Contributor Author

nical commented Aug 26, 2023

@gents83 yes, BGLs may now conceptually hold a strong reference to another compatible BGL. They need to keep the BGL they point to alive so in terms of arcanization they probably need to hold an arc.

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.

6 participants