MinecraftForge / SrgUtils

A library for working with SRG, and other mapping variants.
GNU Lesser General Public License v2.1
12 stars 16 forks source link

Synchronize puts on Caches #5

Closed AterAnimAvis closed 2 years ago

AterAnimAvis commented 2 years ago

Encountered this whilst using FG5 / FART

FART uses SrgUtils in an asynchronize manner (via the AsyncHelper -> RenamingTransformer -> EnhancedRemapper -> MappingFile.remapClass).

Usage in such a manner can hit the HashMap internal optimization threading issue.

This PR migrates the caches to use ConcurrentHashMap. This PR synchronizes the puts to the caches.

Stacktrace ``` > Configure project : Currently on non-main branch: dev-cleanup Mod version: 0.1.17-alpha-g8d22283-dev-cleanup Java: 16 JVM: 16+36(AdoptOpenJDK) Arch: amd64 [20:45:01] [main/INFO]: Writing debug log file accesstransform.log [20:45:01] [main/INFO]: Access Transformer processor running version 8.0.5+67+master.d58c7dfe [20:45:01] [main/INFO]: Command line arguments [--inJar, C:\Users\USER\.gradle\caches\forge_gradle\minecraft_user_repo\net\minecraftforge\forge\1.16.5-36.1.16\forge-1.16.5-36.1.16-injected.jar, --outJar, C:\Users\USER\.gradle\caches\forge_gradle\minecraft_user_repo\net\minecraftforge\forge\1.16.5-36.1.16_mapped_official_1.16.5\forge-1.16.5-36.1.16_mapped_official_1.16.5.jar, --logFile, accesstransform.log, --atFile, $WORKSPACE$\build\_atJar_5\parent_at.cfg] [20:45:01] [main/INFO]: Reading from C:\Users\USER\.gradle\caches\forge_gradle\minecraft_user_repo\net\minecraftforge\forge\1.16.5-36.1.16\forge-1.16.5-36.1.16-injected.jar [20:45:01] [main/INFO]: Writing to C:\Users\USER\.gradle\caches\forge_gradle\minecraft_user_repo\net\minecraftforge\forge\1.16.5-36.1.16_mapped_official_1.16.5\forge-1.16.5-36.1.16_mapped_official_1.16.5.jar [20:45:01] [main/INFO]: Transformer file $WORKSPACE$\build\_atJar_5\parent_at.cfg [20:45:01] [main/WARN]: Found existing output jar C:\Users\USER\.gradle\caches\forge_gradle\minecraft_user_repo\net\minecraftforge\forge\1.16.5-36.1.16_mapped_official_1.16.5\forge-1.16.5-36.1.16_mapped_official_1.16.5.jar, overwriting [20:45:06] [main/INFO]: JAR transformation complete C:\Users\USER\.gradle\caches\forge_gradle\minecraft_user_repo\net\minecraftforge\forge\1.16.5-36.1.16_mapped_official_1.16.5\forge-1.16.5-36.1.16_mapped_official_1.16.5.jar Creating SRG -> MCP TSRG Exception in thread "main" java.lang.RuntimeException: Failed to execute task net/minecraft/client/renderer/GPUWarning.class at net.minecraftforge.fart.internal.AsyncHelper.invokeAll(AsyncHelper.java:70) at net.minecraftforge.fart.internal.AsyncHelper.invokeAll(AsyncHelper.java:55) at net.minecraftforge.fart.internal.RenamerImpl.run(RenamerImpl.java:121) at net.minecraftforge.fart.Main.main(Main.java:173) Caused by: java.util.concurrent.ExecutionException: java.lang.ClassCastException at java.base/java.util.concurrent.ForkJoinTask.get(ForkJoinTask.java:1011) at net.minecraftforge.fart.internal.AsyncHelper.invokeAll(AsyncHelper.java:66) ... 3 more Caused by: java.lang.ClassCastException at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method) at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:78) at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45) at java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:499) at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:480) at java.base/java.util.concurrent.ForkJoinTask.getThrowableException(ForkJoinTask.java:608) ... 5 more Caused by: java.lang.ClassCastException: class java.util.HashMap$Node cannot be cast to class java.util.HashMap$TreeNode (java.util.HashMap$Node and java.util.HashMap$TreeNode are in module java.base of loader 'bootstrap') at java.base/java.util.HashMap$TreeNode.moveRootToFront(HashMap.java:1973) at java.base/java.util.HashMap$TreeNode.treeify(HashMap.java:2089) at java.base/java.util.HashMap.treeifyBin(HashMap.java:772) at java.base/java.util.HashMap.putVal(HashMap.java:644) at java.base/java.util.HashMap.put(HashMap.java:612) at net.minecraftforge.srgutils.MappingFile.remapClass(MappingFile.java:113) at net.minecraftforge.fart.internal.EnhancedRemapper.map(EnhancedRemapper.java:88) at org.objectweb.asm.commons.Remapper.mapType(Remapper.java:78) at org.objectweb.asm.commons.Remapper.mapType(Remapper.java:97) at org.objectweb.asm.commons.SignatureRemapper.visitClassType(SignatureRemapper.java:79) at org.objectweb.asm.signature.SignatureReader.parseType(SignatureReader.java:200) at org.objectweb.asm.signature.SignatureReader.parseType(SignatureReader.java:240) at org.objectweb.asm.signature.SignatureReader.accept(SignatureReader.java:122) at org.objectweb.asm.commons.Remapper.mapSignature(Remapper.java:209) at org.objectweb.asm.commons.ClassRemapper.visit(ClassRemapper.java:108) at org.objectweb.asm.ClassReader.accept(ClassReader.java:569) at org.objectweb.asm.ClassReader.accept(ClassReader.java:424) at net.minecraftforge.fart.internal.RenamingTransformer.process(RenamingTransformer.java:54) at net.minecraftforge.fart.internal.EntryImpl$ClassEntry.process(EntryImpl.java:72) at net.minecraftforge.fart.internal.EntryImpl$ClassEntry.process(EntryImpl.java:50) at net.minecraftforge.fart.internal.RenamerImpl.processEntry(RenamerImpl.java:194) at net.minecraftforge.fart.internal.AsyncHelper.lambda$null$2(AsyncHelper.java:54) at java.base/java.util.concurrent.ForkJoinTask$AdaptedCallable.exec(ForkJoinTask.java:1458) at java.base/java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:295) at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1016) at java.base/java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:1665) at java.base/java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1598) at java.base/java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:183) ```
LexManos commented 2 years ago

The better option is to use manual locks when writing instead of having every get have sync costs.

LexManos commented 2 years ago

Better, not able to reproduce the issue people were having, but these locks should be a good balance of thread saftey and speed.