AIOBE in TextureCTM#connectTo 1.18
Closed this issue · 13 comments
In dev for Mekanism I am getting a good number of crashes when joining my world due to an AIOBE in fastutil from CTM's call in TextureCTM#connectTo. It doesn't always crash, but it is happening a fair bit for me.
Crash reports: https://gist.github.com/pupnewfster/c790e8c2bdf41af9d3cf7ebcf2cf73bb
CTM Version: 1.1.3+1
Forge Version: 39.0.63
From a glance at the port commit, the only thing that stands out to me as potentially problematic in places (though maybe it is fine) is changes from .collect(Collectors.toList()) to .toList() as .toList() returns an immutable list, though without looking closer at the use cases of various methods (which is a pain to do on github) I can't say if any of the places that changed is liable to cause issues and assumes it is a mutable list. I only noticed this because in the TextureCTM class there is a change like this except given it is in a different method I somewhat doubt it has to do with it.
maybe related to vigna/fastutil#246 which could indicate a threading issue
would you mind sharing the current state of mekanism and a save file where this is happening, as I didn't encounter it, while porting the mod and couldn't reproduce it
I can but it is a bit inconsistent, it happens a lot more frequently if after joining the world I tab out while it is still loading so it starts in the pause menu, and then I hit back to game.
Some information about this issue:
vigna/fastutil#246 appears to be related, and the solution is "don't do multithreading".
Minecraft 1.18 added multithreaded chunk meshing as indicated by changelog https://www.minecraft.net/en-us/article/minecraft-snapshot-21w37a under the section PRIORITY UPDATE SETTING
This was confirmed, as on my system at least 2 threads tried to access this method, with the default setting being "threaded" in all used testing.
I couldn't reproduce the crash in game, but the same exception is thrown if multiple threads try to put a value into this kind of map.
This didn't happen, after wrapping the map into a synchronized Map by fastutil, so I just wrapped the map in ctm. This has not had a measurable performance impact.
Yes, this is simply a threadsafety issue, I can see how multiple cache maps would be modified concurrently.
@agnor99 Your fix is in the wrong place, cache loading is atomic and initializing the maps isn't where the problem occurs. The issue is below when the map is modified without any threadsafety protections. I'll need to test a few solutions to see what performs best, but the most obvious is simply adding a synch on the map object before calling put.
This fix doesn't synchronize the initiwlixatiom of the map, but synchronizes write operations yo the map if you wan't to look at the impl of synchronize() and the Synchronized map it produces
Right, that makes sense. But I'd rather not synchronize reads unnecessarily, since the vast majority of access will be reads and synch adds a lot of overhead (possibly even so much as to make the caching worthless).
I really wouldn't recommend removing the synchronize on reads, as I was able to cause crashes if I tried to get, while a put operation was running, therefore I would suggest to either fully synchronize it, or remove the cache, performance tests should be done, but my change didn't cause a measurable performance impact even with over 100.000 ctm affected blocks
I am experiencing this same issue when trying to load RFTools dimensions
crash-2022-02-03_09.11.35-server.txt
Just adding to this so i don't add a duplicate report:
Happened when entering a Futurepack dungeon in 1.18.2.
Adding to this again as i'm still hoping and waiting for a fix, i've found this to be more common in bases where there are a lot of connecting textures that use CTM to make their textures
pack version ATM7 0.4.22, ctm version CTM-1.18.2-1.1.4+4.jar
crash-2022-07-26_23.00.29-client.txt
i have several crash reports identical to this if needed.
crash-2022-08-03_21.14.39-client.txt
crash-2022-08-03_23.09.05-client.txt
Unfortunately still a prevalent issue, hope to see a fix for 1.18 in the near future!
CTM-1.18.2-1.1.4+4
Mekanism-1.18.2-10.2.5.465
Fixed by b7a6c01