Batch of fixes for WorkerThreadPool and ResourceLoader (safe set) #94526
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
These deferring measures were added to aid threaded resource loading in being safe. They were removed as seemingly unneeded, but it seems they are needed so resources involved in threaded loading interact with others only after "sync points".
ResourceLoader: - Fix invalid tokens being returned. - Remove no longer written `ThreadLoadTask::dependent_path` and the code reading from it. - Clear deadlock hazard by keeping the mutex unlocked during userland polling. WorkerThreadPool: - Include thread call queue override in the thread state reset set, which allows to simplify the code that handled that (imperfectly) in the ResourceLoader. - Handle the mutex type correctly on entering an allowance zone. CommandQueueMT: - Handle the additional possibility of command buffer reallocation that mutex unlock allowance introduces.
This is about not letting the resource format loader set the error code directly on the task anymore. Instead, it's stored locally and assigned only when it is right to do so. Otherwise, other tasks may see an error code in the current one before it's state having transitioned to errored. While this, besides the technically true data race, may not be a problem in practice, it causes surprising situations during debugging as it breaks assumptions.
|
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.
Extracted from #94169. See discussion there for details.
Re-add resource thread-safety measures
These deferring measures were added to aid threaded resource loading in being safe.
They were removed as seemingly unneeded, but it seems they are needed so resources involved in threaded loading interact with others only after "sync points".
WorkerThreadPool: Fix wrong sync logic breaking task map integrityFixup recent changes to threading concerns
ResourceLoader:ThreadLoadTask::dependent_pathand the code reading from it.WorkerThreadPool:ResourceLoader.CommandQueueMT:ResourceLoader: Fix sync issues with error reportingThis is about not letting the resource format loader set the error code directly on the task anymore. Instead, it's stored locally and assigned only when it is right to do so.
Otherwise, other tasks may see an error code in the current one before it's state having transitioned to errored. While this, besides the technically true data race, may not be a problem in practice, it causes surprising situations during debugging as it breaks assumptions.
bytes_to_var_with_objectsfails to decode custom classes on threads in v4.3.beta3 #94220