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.

Sign up for GitHub

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

Jump to bottom

Remake ResourceCache thread safety code and API #62309

Merged
merged 1 commit into from
Jun 25, 2022
Merged

Remake ResourceCache thread safety code and API #62309

merged 1 commit into from
Jun 25, 2022

Conversation

reduz
Copy link
Member

@reduz reduz commented Jun 22, 2022

  • Ensures thread safety when resources are destroyed.
  • Simplified API by always forcing ResourceCache::get_ref, which needs less hacks and is fully thread safe.
  • Removed RWLock for resources because its not possible to use for the new logic. Should not be a problem.

Supersedes #57533

@reduz
Remake resource thread safety and API
e772b65 
* Ensures thread safety when resources are destroyed.
* Simplified API by always forcing `ResourceCache::get_ref`, which needs less hacks and is fully thread safe.
* Removed RWLock for resources because its not possible to use for the new logic. Should not be a problem.

Supersedes godotengine#57533
@reduz reduz requested review from a team as code owners June 22, 2022 11:49
@reduz reduz changed the title Remake resource thread safety and API Remake ResourceCache thread safety code and API Jun 22, 2022
@akien-mga akien-mga added this to the 4.0 milestone Jun 22, 2022
RandomShaper
RandomShaper approved these changes Jun 22, 2022
View reviewed changes
Copy link
Member

@RandomShaper RandomShaper left a comment

Choose a reason for hiding this comment

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

LGTM, but for full peace of mind, let's wait until someone else can take a look.

@reduz reduz mentioned this pull request Jun 23, 2022
Closed
@akien-mga akien-mga mentioned this pull request Jun 23, 2022
Closed
akien-mga
akien-mga approved these changes Jun 25, 2022
View reviewed changes
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Changes look sensible to me. Would be good to get more reviews as Pedro mentioned, but since this hasn't happened in a few days, it's probably best to merge now and see how it fares under testing.

@akien-mga akien-mga merged commit b192073 into godotengine:master Jun 25, 2022
@akien-mga
Copy link
Member

Thanks!

@volokh0x volokh0x mentioned this pull request Aug 10, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RandomShaper RandomShaper RandomShaper approved these changes

@akien-mga akien-mga akien-mga approved these changes

@neikeq neikeq Awaiting requested review from neikeq

Assignees
No one assigned
Labels
bug topic:core
Projects
None yet
Milestone
4.0
Development

Successfully merging this pull request may close these issues.
3 participants
@reduz @akien-mga @RandomShaper