[1.12] Safe operations to run in seperate threads?

[Kinniken]

Hi all,

 

I've been debugging some occasional ConcurrentAccessException errors I've been occasionally having in my mod, generally thrown up in the Minecraft code like this:

 

java.util.ConcurrentModificationException
	at java.util.HashMap$HashIterator.nextNode(HashMap.java:1442)
	at java.util.HashMap$KeyIterator.next(HashMap.java:1466)
	at net.minecraft.entity.EntityTracker.func_72788_a(EntityTracker.java:290)
	at net.minecraft.server.MinecraftServer.func_71190_q(MinecraftServer.java:777)
	at net.minecraft.server.dedicated.DedicatedServer.func_71190_q(DedicatedServer.java:396)
	at net.minecraft.server.MinecraftServer.func_71217_p(MinecraftServer.java:666)
	at net.minecraft.server.MinecraftServer.run(MinecraftServer.java:524)
	at java.lang.Thread.run(Thread.java:748)

 

I've narrowed it down not to faulty packet handling or GUI updates as I was hoping for, but to either my pathing thread or my "geography analysis" thread. Both do read-only operations on blocks (chunk.getBlockState() mostly), and they do it in separate threads to lessen CPU spikes. In 1.7 that worked fine, in 1.12 it seems to be unsafe. I'm hoping but can't yet prove that the actual issue is that I occasionally force a chunk to load and that this is the unsafe operation (by forcing Minecraft to update its entity list), in which case I could plausibly harden my code to make sure I don't do that.

 

Has anybody done any analysis on this topic? I know Minecraft is horrible at thread-safety, but I'd really like a way to make this work, long path calculations used to cause very annoying CPU spikes before I threaded the whole thing and I'd rather not go back to it.

 

Thanks!

[Draco18s]

Post your code. Like, all of it.

[Kinniken]

Of the mod? 45k lines?

 

And even if you mean just the classes involved, the geography one refers to so many other classes it's impossible to post here, and the pathing one is an A* system that's way more complex than this issue.

 

I'm not asking for help debugging my code anyway, I'm asking for info on what is known regarding safe threading behaviour.

[Kinniken]

Ok, from what I've gathered from other modders, accessing blockstates in loaded chunks is supposedly thread-safe, loading chunks definitely isn't.

 

I've rewritten my code to always check the chunk load status while running threaded code, I'll see if it solves that issue.

[Draco18s]

3 hours ago, Kinniken said:

Of the mod? 45k lines?

GitHub repositories are amazing

[jabelar]

ConcurrentAccessException are often encountered when iterating through Java Collections. Especially if you try to modify the list while iterating. In Minecraft it seems that entities, blocks and such are often being concurrently accessed (I guess for things like rendering and loading and such, although someone more knowledgeable should comment).

 

So basically I'm saying it probably isn't just about loaded chunks but simply the fact that you're iterating and I guess modifying collections that are even being rendered or otherwise accessed elsewhere.

 

Now, I expect Minecraft does some threadsafe locking in order to allow use of the lists (vanilla somehow is able to iterate through them) so maybe someone here can enlighten us on how that works.

Edited February 27, 2018 by jabelar

[Kinniken]

Making copies of entire chunks would kind of defeat the purpose, which is to optimize performance...

 

I've checked with several modders, everyone who worked with threaded access agrees that accessing block states in loaded chunks is thread-safe.

 

I've looked at the Minecraft code, it indeed implies only reading from two hash maps (the one with loaded chunks and the one with block states within a chunk), and read-only operations on maps are thread-safe in Java.

 

I've now updated my mod to validate that a chunk is loaded before doing any reading of block states in it, and since then neither me nor other testers have had those crashes.

 

The only problem I could see is if I did a read access on a chunk that got unloaded between my check and the read. And since I do those two operations immediately one after the other that seems like a negligible risk.

 

Unless players start reporting threading crashes again I'm considering this closed.

[Kinniken]

On 27/02/2018 at 5:58 AM, jabelar said:

So basically I'm saying it probably isn't just about loaded chunks but simply the fact that you're iterating and I guess modifying collections that are even being rendered or otherwise accessed elsewhere.

The problem was not me directly modifying collections, the problem was me forcing Minecraft to load chunks by trying to read block states in them, and doing that can modify collections - the entity list in particular, since loading a chunk causes entities that were in it to be loaded. So yes, even if your own code does nothing to do with collections or entities, chunk loading is definitely not thread-safe.

 

And Minecraft does basically no threadsafe locking, which is why using threaded code in Minecraft is dangerous

[Kinniken]

Regarding the fact that I'm not guaranteed to see an updated value after a write operation, you are right, that's a limit of how maps work in Java. However that's not an issue in my use case - sure a path I'm calculating could be made obsolete by a block being placed 5 ms before I run my calculation, but it also could be made obsolete by a block placed 5 ms after. That's why paths need to be recalculated periodically, or in case an entity gets stuck.

 

For the issue of reading partially-saved data, as far as I can see in the MC code, the bytes that actually represent a block and its meta value are set and read together, sequentially. So the potential problem would be for me to do a read while only part of the four bytes that represent a block state have been saved. Not impossible, but...

 

Against that, if I want to calculate a path between coords say 100 blocks away, I need to analyse an area of something like 7*7 chunks. I'm willing to time it, but the memory and CPU usage of duplicating 49 chunks to calculate a path doesn't seem that trivial.

 

Anyway, I'm not going to stick to "5 minutes of testing", I have enough testers running the mod for hours that if threading crashes do reoccur I'll know about it.

[Draco18s]

10 hours ago, diesieben07 said:

Doing a 5mb memory copy is (again I am estimating here, I am not in the mood to look up exact figures) a sub millisecond task on a machine that's even a few years old.

I once did a speed analysis of comparing block states on every block in a chunk from 0 to the surface (so that same average of 90) and when no modifications needed to occur (e.g. none of the blocks I was looking for existed, yes, I was looking for one of many blocks, so I was doing some List#indexOf()) it took around 400,000 nanos (or 0.4ms). When the block(s) I was looking for did exist and I had to execute some block updates elsewhere (frequently in another chunk entirely) that number rose...to 600,000 nanos.

 

So your guess isn't wrong.

Edited March 1, 2018 by Draco18s

[Kinniken]

Ok, that is faster than I'd have thought. Still, that's a lot of extra RAM usage, and more importantly, a fairly complicated new dev with strong adherence to yet more Minecraft classes, meaning yet another bit of code that can go wrong in upgrades.

 

Thanks for the detailed information. Should my current approach prove problematic for players, I'll look into doing a copy from the main thread instead.

[Kinniken]

5 MB per path. On a server with several active villages with say 20 villagers each, it adds up fast. Plus my other threaded uses.

 

You're right though, my setup is also vulnerable to Minecraft upgrades, but much less so. Ultimately my adherence is only to one method testing whether a chunk is loaded or not, not to the detailed chunk implementation that making copies would require.

 

And I'm not ignoring the problem, I'm making a choice with a know risk that has a very, very low occurrence, that of reading four bytes of sequential data and assuming there won't be a write operation in the meantime.

 

Anyway, thanks again for the technical help, but ultimately I have to prioritise my development work, and unless players start having threaded problems again I'm considering this issue closed.