-
-
Notifications
You must be signed in to change notification settings - Fork 20.8k
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
[4.3] Cherry-picks related to ResourceLoader
#96606
Open
RandomShaper
wants to merge
17
commits into
godotengine:4.3
Choose a base branch
from
RandomShaper:res_loader_cherrypicks_4.3
base: 4.3
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
[4.3] Cherry-picks related to ResourceLoader
#96606
RandomShaper
wants to merge
17
commits into
godotengine:4.3
from
RandomShaper:res_loader_cherrypicks_4.3
+480
−284
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
(cherry picked from commit 2ff6594)
(cherry picked from commit bd0959e)
Benefits: - Simpler code. The main load function is renamed so it's apparent that it's not just a thread entry point anymore. - Cache and thread modes of the original task are honored. A beautiful consequence of this is that, unlike formerly, re-issued loads can use the resource cache, which makes this mechanism much more performant. - The newly added getter for caller task id in WorkerThreadPool allows to remove the custom tracking of that in ResourceLoader. - The check to replace a cached resource and the replacement itself happen atomically. That fixes deadlock prevention leading to multiple resource instances of the same one on disk. As a side effect, it also makes the regular check for replace load mode more robust. (cherry picked from commit 28619e2)
(cherry picked from commit 5c970db)
1. Make handling of user tokens atomic: Loads started with the external-facing API used to perform a two-step setup of the user token. Between both, the mutex was unlocked without its reference count having been increased. A non-user-initiated load could therefore destroy the load task when it unreferenced the token. Those stages now happen atomically so in the one hand, the described race condition can't happen so the load task life insurance doesn't have a gap anymore and, on the other hand, the ugliness that the call to load could return `ERR_BUSY` if happening while other thread was between both steps is gone. The code has been refactored so the user token concerns are still outside the inner load start function, which is agnostic to that for a cleaner implementation. 2. Clear ambiguity between load operations running on `WorkerThreadPool`: The two cases are: single-loaded thread directly started by a user pool task and a load started by the system as part of a multi-threaded load. Since ensuring all the code dealing with this distinction would make it very complex, and error-prone, a different measure is applied instead: just take one of the cases out of the dicotomy. We now ensure every load happening on a pool thread has been initiated by the system. The way of achieving that is that a single-threaded user-started load initiated from a pool thread, is run as another task. (cherry picked from commit df23858)
This fixes a rare but possible deadlock, maybe due to undefined behavior. The new implementation is safer, at the cost of some added boilerplate. (cherry picked from commit f4d7685)
(cherry picked from commit 31a9e10)
(cherry picked from commit 9cbc3f1)
(cherry picked from commit 0441c67)
This was referenced Sep 5, 2024
Merged
RandomShaper
force-pushed
the
res_loader_cherrypicks_4.3
branch
from
September 6, 2024 06:58
b857207
to
a38994b
Compare
RandomShaper
force-pushed
the
res_loader_cherrypicks_4.3
branch
from
September 6, 2024 15:50
a38994b
to
98c1328
Compare
RandomShaper
force-pushed
the
res_loader_cherrypicks_4.3
branch
2 times, most recently
from
September 6, 2024 16:32
971c10a
to
2f83402
Compare
RandomShaper
force-pushed
the
res_loader_cherrypicks_4.3
branch
from
September 13, 2024 10:20
e0e5a8c
to
8124bbf
Compare
RandomShaper
force-pushed
the
res_loader_cherrypicks_4.3
branch
from
September 17, 2024 09:59
8124bbf
to
c41af7e
Compare
RandomShaper
force-pushed
the
res_loader_cherrypicks_4.3
branch
from
September 23, 2024 16:56
c41af7e
to
9ed06bc
Compare
Co-authored-by: Summersay415 <[email protected]> (cherry picked from commit 214deab)
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.
In order to be sure everything worth cherry-picking into 4.3 is included, and also to help a bit with the correct order and conflict solving, I'm making this PR, cherry-picking the following 4.4-targeting PRs (in this exact order):
ConditionVariable
inResourceLoader
#94801WorkerThreadPool
andResourceLoader
#94169-Wundefined-var-template
Clang warning in SafeBinaryMutex #96108Return error when no ResourceFormatLoader found (reverted) #955082024-09-23:Removed (reason: Return error when no ResourceFormatLoader found (reverted) #95508 (comment))verbose_stdout
is enabled #94920ResourceLoader: Fixup resource changed feature #96656 🆕🆕2024-09-17: Removed (reason: Add optional count argument toSemaphore::post
#93605 (comment)).