Skip to content
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

AIOBE in TextureCTM#connectTo 1.18 #174

Closed
pupnewfster opened this issue Jan 30, 2022 · 13 comments
Closed

AIOBE in TextureCTM#connectTo 1.18 #174

pupnewfster opened this issue Jan 30, 2022 · 13 comments

Comments

@pupnewfster
Copy link
Contributor

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.

@justliliandev
Copy link
Contributor

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

@justliliandev
Copy link
Contributor

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

@pupnewfster
Copy link
Contributor Author

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.

justliliandev added a commit to justliliandev/ConnectedTexturesMod that referenced this issue Feb 1, 2022
see Chisel-Team#174 for more information
@justliliandev
Copy link
Contributor

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.

@tterrag1098
Copy link
Member

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.

@justliliandev
Copy link
Contributor

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

@tterrag1098
Copy link
Member

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

@justliliandev
Copy link
Contributor

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

@aAndrew3030
Copy link

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

@Darkfiend009
Copy link

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.

@Mitchell5200
Copy link

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.

@KuwuroUsagi
Copy link

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

@tterrag1098
Copy link
Member

Fixed by b7a6c01

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants