ConnectedTexturesMod

ConnectedTexturesMod

110M Downloads

AIOBE in TextureCTM#connectTo 1.18

pupnewfster opened this issue · 13 comments

commented

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.

commented

maybe related to vigna/fastutil#246 which could indicate a threading issue

commented

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

commented

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.

commented

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.

commented

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.

commented

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

commented

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).

commented

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

commented

I am experiencing this same issue when trying to load RFTools dimensions
crash-2022-02-03_09.11.35-server.txt

commented

Just adding to this so i don't add a duplicate report:

https://pastebin.com/PnxqVbFr

Happened when entering a Futurepack dungeon in 1.18.2.

commented

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

commented

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.

commented

Fixed by b7a6c01