Avoid multiple possibilites of deadlock in resource loading #77143
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
RandomShaper
force-pushed
the
fix_wtp_deadlocks
branch
from
11cf316 to
a6e43f7
Compare
akien-mga
reviewed
May 17, 2023
| @@ -100,10 +100,13 @@ | |||
| </description> | |||
| </method> | |||
| <method name="wait_for_task_completion"> | |||
| <return type="void" /> | |||
| <return type="int" enum="Error" /> | |||
There was a problem hiding this comment.
Technically this breaks compatibility. Needs to be assessed whether it's worth making an exception and allow it.
There was a problem hiding this comment.
I can change it so the exposed API doesn't change. However, I don't really see the downside. If the caller is not interested, they can just ignore the returned value. After all, there's no behavior change. This gives an opportunity to the user of doing something about the deadlock, but the possibility of it occuring was already there.
There was a problem hiding this comment.
I think its fine, It does not change anything for GDScript and for GDExtension we already have the compatibility fallback system.
reduz
approved these changes
May 17, 2023
|
Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Multi-threaded resource loading can deadlock if one of the current load tasks are waiting for another which hasn't still started running to complete and all the low-priority slots in the
WorkerThreadPoolare occupied. This PR allows theWorkerThreadPoolto detect such a situation and schedule additional low-prio tasks until the situation is solved.Since the
WorkerThreadPoolneeds to count how many low-prio threads are awaiting on others, this PR also needed to add support for multiple waiters. Otherwise, theResourceLoaderwould use its own waiting mechanism for tasks already awaited, which would prevent such count from being exhaustive.Another possibility of deadlock is that a worker thread in the
WorkerThreadPool, upon one task waiting for another, runs yet another task over the stack frame of the awaiting one. That can lead to a load task eventually waiting for a previous one which can no longer finish because it's "buried" in the stack. Many possibilites have been assessed and discarded (enrich the worker pool with some task dependency info, avoid burying tasks' stack frames and even adding support for fibers for easy task switching within a given thread). The approach here is let theWorkerThreadPooldetect the fact and reportERR_BUSYto the caller. In this PR theResourceLoaderis trained to catch the problem and apply a workaround.This PR also ensures there's always at least one high-priority thread in the pool.