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

Implement a missing part of bind group layout deduplication #4200

Merged
merged 3 commits into from
Oct 4, 2023

Conversation

nical
Copy link
Contributor

@nical nical commented Oct 3, 2023

Checklist

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

Connections

Description

render_pipeline_get_bind_group_layout should do bind group layout de-duplication, but it currently does not.

It will probably help the review to consider the two commits separately:

  • The first one changes the way duplicate bind group layouts are stored: most of the data is only ever stored in the case of the "leaf" layouts (not duplicates), while duplicates mostly contain the ID to forward to. Before this commit we would create a real raw bind group layout for duplicates (but then using it would be a bug since we always want to use the one the duplicate points to). In retrospect I should have done that in the initial BGL de-duplication PR, as it avoids creating resources unnecessarily and removes the possibility for making mistakes. This makes the second commit easier to write:
  • The second commit implements the missing bit of bind group layout de-duplication in render_pipeline_get_bind_group_layout. The latter must create a new (duplicate) BGL if the ID is provided externally but was missing that part before this commit.

I wrote this with the assumption that the MultiRefCount should be zero at the creation of a bind group layout, I believe it is correct but it would not hurt to look at it with a critical eye during the review.

@gents83 This PR will probably collide a bit with arcanization as most do I assume. The main thing that matters for arcanization is the change in the second commit where instead of creating a new BGL, a duplicate that references an existing one is created, similarly to what was done in #3925

Testing

That's covered by the CTS in Firefox, but unfortunately tedious to test here because wgpu does not exercise this code path.

@nical nical requested a review from a team as a code owner October 3, 2023 13:50
@gents83
Copy link
Contributor

gents83 commented Oct 3, 2023

Do you think we can wait to do this after Arcanization or do you prefer me to re-implement/re-check it after your submit?

@nical
Copy link
Contributor Author

nical commented Oct 3, 2023

Do you think we can wait to do this after Arcanization or do you prefer me to re-implement/re-check it after your submit?

It blocks some important stuff in firefox and our deadlines are getting pretty tight, so we can wait for a few days but not much longer (we actually held off #3925 for about 6 weeks because we thought arcanization was about to land, but as you know it's a difficult beast to push over the finish line!).

@gents83
Copy link
Contributor

gents83 commented Oct 3, 2023

Oki, this week I'll be away for work, so I'll be able to be back on this during the weekend.

If you need to go on, no problem, I'll catch up 👍🏼

@ErichDonGubler ErichDonGubler self-assigned this Oct 3, 2023
@ErichDonGubler ErichDonGubler self-requested a review October 3, 2023 14:58
@ErichDonGubler
Copy link
Member

@gents83: Does this actually look like a significant amount of effort to adjust arcanization for? I don't think there are any significant changes to how tracking with BindGroupLayout::multi_ref_count was used here. This is mostly a refactor to use an enum representation for the guts of BGLs.

Copy link
Member

@ErichDonGubler ErichDonGubler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Crazily enough (given my newness to wgpu), I actually feel confident in approving on this. @nical onboarded me to his solution synchronously yesterday, which helped a great deal. 🙂

Left mostly non-blocking feedback. For most code style nitpicks, I left fixup! commits. Gonna trust @nical to address feedback before merging, and leave a green checkmark here for now.

wgpu-core/src/binding_model.rs Show resolved Hide resolved
wgpu-core/src/binding_model.rs Outdated Show resolved Hide resolved
wgpu-core/src/binding_model.rs Outdated Show resolved Hide resolved
wgpu-core/src/binding_model.rs Show resolved Hide resolved
wgpu-core/src/binding_model.rs Show resolved Hide resolved
wgpu-core/src/device/resource.rs Show resolved Hide resolved
wgpu-core/src/device/life.rs Outdated Show resolved Hide resolved
wgpu-core/src/device/resource.rs Outdated Show resolved Hide resolved
wgpu-core/src/device/resource.rs Outdated Show resolved Hide resolved
wgpu-core/src/device/resource.rs 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.

Looks good :)

@cwfitzgerald
Copy link
Member

@nical merge when ready, don't want to merge early if you're not done adjusting.

@nical nical merged commit 32b761a into gfx-rs:trunk Oct 4, 2023
23 checks passed
@nical nical deleted the bgl-dedup-2 branch October 4, 2023 09:12
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.

4 participants