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

Clean up weak references to texture views and bind groups #5595

Merged

Conversation

xiaopengli89
Copy link
Contributor

@xiaopengli89 xiaopengli89 commented Apr 24, 2024

Description

Cleanup weak references to texture views and bind groups to prevent leak.

image

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.

@xiaopengli89 xiaopengli89 requested a review from a team as a code owner April 24, 2024 07:34
@xiaopengli89 xiaopengli89 changed the title Clean up weak references to texture views Clean up weak references to texture views and bind groups Apr 24, 2024
@cwfitzgerald
Copy link
Member

Tagging jimb on this as he's most aware of the lifetime management in wgpu-core

@xiaopengli89 xiaopengli89 force-pushed the cleanup-weak-ref-to-texture-view branch 2 times, most recently from 1c964e5 to 977fb27 Compare April 28, 2024 05:08
@cwfitzgerald cwfitzgerald added the PR: needs back-porting PR with a fix that needs to land on crates label Apr 29, 2024
@jimblandy jimblandy self-assigned this Apr 30, 2024
Copy link
Member

@jimblandy jimblandy left a comment

Choose a reason for hiding this comment

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

Thank you for tracking this down! Forgetting to ever actually trim those vectors was sloppy. I really hate backlinks like that, because they always seem to run into problems like this.

However, I think we can do a bit better.

wgpu-core/src/device/life.rs Outdated Show resolved Hide resolved
@jimblandy
Copy link
Member

jimblandy commented May 1, 2024

@xiaopengli89 I've pushed a commit that makes the change I suggested. Could you look it over and see if it seems okay, and verify that it does fix the leak?

@xiaopengli89
Copy link
Contributor Author

@xiaopengli89 I've pushed a commit that makes the change I suggested. Could you look it over and see if it seems okay, and verify that it does fix the leak?

@jimblandy LGTM.

@xiaopengli89 xiaopengli89 force-pushed the cleanup-weak-ref-to-texture-view branch from c7166d7 to 500804e Compare May 6, 2024 03:27
@jimblandy jimblandy merged commit d5d683d into gfx-rs:trunk May 6, 2024
25 checks passed
@xiaopengli89
Copy link
Contributor Author

xiaopengli89 commented May 7, 2024

@jimblandy Sorry, we cannot use the change because suspected_resources only contains resources whose user handle has died.

Wumpf pushed a commit to Wumpf/wgpu that referenced this pull request May 18, 2024
* Clean up weak references to texture views

* add change to CHANGELOG.md

* drop texture view before clean up

* cleanup weak ref to bind groups

* update changelog

* Trim weak backlinks in their holders' triage functions.

---------

Co-authored-by: Jim Blandy <jimb@red-bean.com>
@Wumpf Wumpf mentioned this pull request May 18, 2024
6 tasks
ErichDonGubler pushed a commit that referenced this pull request May 18, 2024
* Clean up weak references to texture views

* add change to CHANGELOG.md

* drop texture view before clean up

* cleanup weak ref to bind groups

* update changelog

* Trim weak backlinks in their holders' triage functions.

---------

Co-authored-by: Jim Blandy <jimb@red-bean.com>
jimblandy added a commit to jimblandy/wgpu that referenced this pull request May 23, 2024
* Clean up weak references to texture views

* add change to CHANGELOG.md

* drop texture view before clean up

* cleanup weak ref to bind groups

* update changelog

* Trim weak backlinks in their holders' triage functions.

---------

Co-authored-by: Jim Blandy <jimb@red-bean.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: needs back-porting PR with a fix that needs to land on crates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants