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

ResourceLoader: Let resource setup late steps invoke loading in turn #94910

Merged
merged 1 commit into from
Jul 31, 2024

Conversation

RandomShaper
Copy link
Member

@RandomShaper RandomShaper commented Jul 29, 2024

The low-level issue in #94586 that the BinaryMutex was being tried to be recursively relocked.

Fixes #94586.

AThousandShips
AThousandShips previously approved these changes Jul 29, 2024
Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

This makes sense to me though TIWAGOS, to me especially the load_task.cond_var->notify_all(); is a strong indicator that it should be unlocked at that point based on how condition variables work (realized this wasn't necessarily relevant either way, as there's nothing wrong with notifying while still locked, or while not locked, just affects some timing)

But my TIWAGOS would mainly apply to if there aren't any critical sections at all in that part, that much I can't speak to

@RandomShaper RandomShaper force-pushed the res_load_unlocked branch 2 times, most recently from 6da23af to 76ac8b3 Compare July 29, 2024 16:08
@RandomShaper
Copy link
Member Author

Thanks to heaven, you smelled something here. I added the lock-unlock too early! I've fixed it. It now happens once the last write has been done, because we don't want other threads to see the state of the task until we're done writing it. It's only afterwards that we enter that last "read-only" segment of code.

@RandomShaper
Copy link
Member Author

Pushed again since the failed loading branch of the if needed it's own handling of the lock. Please re-review because I'm no longer trusting my eyes in this one. 😅

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.

Cautiously approving as I don't really understand that code, but know RandomShaper does. More eyes welcome on this, would probably merge soon to include this in 4.3 RC2.

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

Not familiar with the details in this section but nothing looks obviously weird here, and these parts don't necessarily look like a critical section so should be safe, but TIWAGOS

@akien-mga akien-mga merged commit 372b3f8 into godotengine:master Jul 31, 2024
18 checks passed
@akien-mga
Copy link
Member

Thanks!

@RandomShaper RandomShaper deleted the res_load_unlocked branch July 31, 2024 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Editor freezes when reimporting glb if mesh has a shader material that contains an #include
3 participants