Use a variable sized thread pool in AssetManager #5153
+2
−1
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
The getAssetAsynchronous method uses a thread pool so that requests to load assets don't block networking threads etc. It has been a single thread pool through the existing git history but it is assumed that it is like this because it used to have a thread and then was converted to use a single thread fixed thread pool. Unfortunately having a single thread only means that if an asset load callback indirectly loads another asset then this can deadlock since the asset loader thread needs to be released before the load request can start, which it can't because that thread is waiting for the result of loading the new asset, thus it deadlocks if an AddOn includes a JavaScript onInit script since it deadlocks waiting to load that script. Changing the thread pool so that the threads load in parallel is an observable change that could cause problems, but if the AddOn isn't already loaded the addonLoader's thread pool loads it instead and that works fine, so MapTool should be alright with addons and other assets loading in parallel.
fishface60
force-pushed
the
release-1.16-addon-load-hang
branch
from
938767b to
bce0ff5
Compare
fishface60
changed the title
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.
This is the version of this patch targeting 1.16, for the version targeting 1.17 see #5151
Identify the Bug or Feature request
fixes #4962
Description of the Change
AssetManager has a thread pool for loading assets that prevents the network handler thread from blocking when it receives a message that requires an asset load.
It is a single thread, which effectively serialises loading.
Unfortunately this means that if the appropriate action after loading an asset is to load a subsequent asset (such as AddOns loading the onInit script in order to run them when the AddOn has been loaded) then this deadlocks.
If the asset is not already cached then instead of the load callbacks being called in the loader thread the assetLoader class' thread pool handles the callbacks, so it should be safe to use a larger thread pool to load assets.
This patch switches the single thread pool with a reference to the
ForkJoinPool.commonPool()which is used by CompletableFutures as the "default" thread pool.Possible Drawbacks
Supporting parallel loading of assets could potentially have observable changes that cause other bugs, but there are circumstances already that allow assets to load in parallel, so these code paths aren't untested.
Release Notes
This change is