This is not a bug but rather a code discussion. I don't know the reasoning behind design choices, so there might be a very good reason for the code being the way it is. Also, I do not have any numbers on the performance impact of loading chunks from disk asynchronously, so the effect might be quite negligible. Moreover, the impact is probably further hidden by the fact that tasks on the main thread are spread over multiple ticks if necessary, and by some caching that is currently already done, so the impact probably isn't that strong.
Nevertheless, I was quite surprised when I saw that chunk loading from disk is not really done off-thread, but effectively on the main thread, and wondered whether this was some kind of oversight or if there is some deeper reason for the way it is. The rest of the code really feels like it should be done properly off-thread and the change feels simple, so I think it is worth pointing out.
As starting point for chunk loading from disk we want to look at
net.minecraft.server.world.ThreadedAnvilChunkStorage.java
private CompletableFuture<Either<Chunk, ChunkHolder.Unloaded>> loadChunk(ChunkPos pos) {
return CompletableFuture.supplyAsync(() -> {
try {
this.world.getProfiler().visit("chunkLoad");
CompoundTag compoundTag = this.getUpdatedChunkTag(pos);
...
return Either.left(new ProtoChunk(pos, UpgradeData.NO_UPGRADE_DATA));
}, this.mainThreadExecutor);
}which produces a CompletableFuture and schedules its computation on the main thread.
The chunk is now loaded from disk by this.getUpdatedChunkTag(pos) which after some method calls basically boils down to the following method
net.minecraft.world.storage.StorageIoWorker.java
@Nullable
public CompoundTag getNbt(ChunkPos pos) throws IOException {
CompletableFuture completableFuture = this.run((completableFuturex) -> {
return () -> {
StorageIoWorker.Result result = (StorageIoWorker.Result)this.results.get(pos);
if (result != null) {
completableFuturex.complete(result.nbt);
} else {
try {
CompoundTag compoundTag = this.storage.getTagAt(pos);
completableFuturex.complete(compoundTag);
} catch (Exception var5) {
LOGGER.warn("Failed to read chunk {}", pos, var5);
completableFuturex.completeExceptionally(var5);
}
}
};
});
try {
return (CompoundTag)completableFuture.join();
} catch (CompletionException var4) {
if (var4.getCause() instanceof IOException) {
throw (IOException)var4.getCause();
} else {
throw var4;
}
}
}While the loading technically runs on a worker thread, the main thread has to wait for the whole duration since it is joined on the resulting CompletableFuture.
This makes me wonder why the CompletableFuture is explicitly joined, rather than simply returning it and then chaining the other actions on it, which feels much more natural. This would then load the chunk properly off-thread without the main thread having to idle for the whole time.
As I said, I have no idea whether this is some oversight or whatever. Feel free to close the report if this is working as intended 🙂
Best,
PhiPro
Comments
migrated
PhiPro
The method you describe already exists and is way higher up the chunk loading chain
net.minecraft.server.world.ServerChunkManager.java
@Nullable
public Chunk getChunk(int x, int z, ChunkStatus leastStatus, boolean create) {
if (Thread.currentThread() != this.serverThread) {
return (Chunk)CompletableFuture.supplyAsync(() -> {
return this.getChunk(x, z, leastStatus, create);
}, this.mainThreadExecutor).join();
} else {
...
CompletableFuture<Either<Chunk, ChunkHolder.Unloaded>> completableFuture = this.getChunkFuture(x, z, leastStatus, create);
this.mainThreadExecutor.runTasks(completableFuture::isDone);
chunk = (Chunk)((Either)completableFuture.join()).map((chunkx) -> {
return chunkx;
}, (unloaded) -> {
if (create) {
throw (IllegalStateException)Util.throwOrPause(new IllegalStateException("Chunk not there when requested: " + unloaded));
} else {
return null;
}
});
this.putInCache(l, chunk, leastStatus);
return chunk;
}
}However, the chunk ticket created in this.getChunkFuture(x, z, leastStatus, create) only lasts for the current tick, hence the chunk will be unloaded again immediately in the next tick. Furthermore, this ticket is only strong enough to load the chunk as border chunk, but not as ticking chunk.
Anyway, this doesn't explain why something is already joined in the very early loading stage. Note further that the main thread can still execute scheduled tasks while waiting in getChunk(...), so it can at least do something useful, as long as there are unfinished tasks, but it is completely frozen while waiting in getNbt(...).
[Mod] Jingy
This is a feature request
There needs to be SOME synchronous chunk loading. There's still a major bug where redstone doesn't load chunks properly. For that to happen, it's necessary for the main thread to demand chunk loading and wait on it.