Async chunk loading for spigot/paper 1.21.3/1.21.4 #4188
Conversation
| } | ||
| SerializableChunkData chunkData = SerializableChunkData.a(cw.getHandle(), c); //SerializableChunkData.copyOf | ||
| NBTTagCompound nbt = chunkData.a(); // SerializableChunkData.write | ||
| return parseChunkFromNBT(new NBT.NBTCompound(nbt)); |
There was a problem hiding this comment.
Kinda missed a bit of the impl description, up to SerializableChunkData.a(cw.getHandle(), c) needs to be executed on main thread, the part bellow it should be moved into the supplier so it would be executed async
Basically the cf should return SerializableChunkData and not the parsed chunk
There was a problem hiding this comment.
But why? I can see no obvious reason to not load in on the parallel thread. No need to stress the main thread here.
There was a problem hiding this comment.
It's saving data that is really not liking concurrent access, and accessing chunk in async in Minecraft is always calling for issues
| protected Supplier<GenericChunk> loadChunkAsync(DynmapChunk chunk){ | ||
| CraftWorld cw = (CraftWorld) w; | ||
| CompletableFuture<GenericChunk> genericChunk = cw.getHandle().m().a.d(new ChunkCoordIntPair(chunk.x, chunk.z)) // WorldServer.getChunkSource().chunkMap.read(new ChunkCoordIntPair(chunk.x, chunk.z)) | ||
| .thenApply(opt -> opt.map(NBT.NBTCompound::new).map(this::parseChunkFromNBT).orElse(null)); |
There was a problem hiding this comment.
Move this to supplier to make sure it's not gonna be executed on main thread
There was a problem hiding this comment.
This is not how async and cf work. All operations chained with "then" will get executed on the thread the load itself is executed. So basically the return value is not that elegant. In theory the cf itself should be returned, but we are bound to the given interface. So moving this to the supplier will result in getting it executed async when the supplier is requested, but why wait when we can start loading it right now?
There was a problem hiding this comment.
All operations chained with "then" will get executed on the thread the load itself is executed.
Yep, and that's what you should avoid, it's not a thread that you can say with confidence that you can stress with additional load. And this is the reason why you should move it to supplier, as you'll for sure be in async thread pool that you can load with stuff like parsing the chunk
|
I loaded this on my server, and it's working fine. Although, I've never had major performance issues, so I can't really compare a before -> after. mspt seemed around the same, still generally low. My server is Spigot 1.21.4, which confirms it works there too (Spigot didn't have async chunk loading before). |
This PR brings back async chunk loading for spigot/paper 1.21.3/1.21.4 requested by #4185.
Basically the impl is described here:
#4164 (comment)
Tested on paper 1.21.4.
Let me know if anything else is needed.