This is again rather a code discussion than an actual bug report. I do not have any specific numbers or examples of this happening, but it can clearly have a negative impact on chunk loading performance, which can be easily avoided. My proposed solution is easy, so I think it is worth mentioning this issue.
Basically, the issue is that worldgen tasks cannot be canceled once scheduled to the worldgen task queue, which can cause clogging if players move around too fast.
As mentioned in MC-177685, the ChunkTicketManager
does some artificial throttling to limit the number of chunk tickets for which the respective chunk is not fully loaded yet. If the player moves away again (before the loading is complete), the chunk ticket and hence the throttling is removed; however, all the already scheduled worldgen tasks (for which there can be many per single chunk ticket) are not canceled and will still be executed. So, if a player moves around too fast and loads and unloads many chunks (especially in multiplayer environments where there are many players), this can lead to the worldgen task queue becoming clogged by tasks that are not needed anymore. Due to MC-177685, this queue is linear and not prioritized, so the old pending tasks indeed take precedence over new important ones.
Currently, when a ChunkHolder
level is demoted, all the pending futures above the new level will be completed with Unloaded
, which does however not abort the underlying tasks associated to them directly. In fact, this does abort all but the lowest level pending task per chunk, since all higher level tasks will receive an Unloaded
chunk, so they will abort once they get executed. The lowest level task can however not be canceled. So in particular chunk loading from disk will not be canceled, which is always present even if the chunks are already generated but not loaded.
I propose that worldgen tasks (and chunk loading from disk) should as first operation check if the associated ChunkHolder
still has a sufficiently high level to execute the task and otherwise abort early.
It is important that this operation is not (only) performed when creating the futures, but also when actually processing the tasks associated to them which do the actual work.
In particular, it is not so important to add such a check in
net.minecraft.server.world.ThreadedAnvilChunkStorage.java
public CompletableFuture<Either<Chunk, ChunkHolder.Unloaded>> createChunkFuture(ChunkHolder chunkHolder, ChunkStatus chunkStatus) {
ChunkPos chunkPos = chunkHolder.getPos();
if (chunkStatus == ChunkStatus.EMPTY) {
return this.loadChunk(chunkPos);
} else {
CompletableFuture<Either<Chunk, ChunkHolder.Unloaded>> completableFuture = chunkHolder.createFuture(chunkStatus.getPrevious(), this);
return completableFuture.thenComposeAsync((either) -> {
...
if (chunk.getStatus().isAtLeast(chunkStatus)) {
CompletableFuture completableFuture2;
if (chunkStatus == ChunkStatus.LIGHT) {
completableFuture2 = this.generateChunk(chunkHolder, chunkStatus);
} else {
completableFuture2 = chunkStatus.runNoGenTask(this.world, this.structureManager, this.serverLightingProvider, (chunkx) -> {
return this.convertToFullChunk(chunkHolder);
}, chunk);
}
this.worldGenerationProgressListener.setChunkStatus(chunkPos, chunkStatus);
return completableFuture2;
} else {
return this.generateChunk(chunkHolder, chunkStatus);
}
}, this.mainThreadExecutor);
}
}
which does not do much work on its own (other than maybe this.convertToFullChunk(...)
). But rather add the check inside the futures created by this.loadChunk(...)
and this.generateChunk(...)
which do the actual work.
Note that the task associated to the future created in this.generateChunk(...)
does not run on the main thread, but on the worldgen thread (the task associated to this.loadChunk(...)
currently does run on the main thread due to MC-177729), so this proposed check accesses ChunkHolder
concurrently.
Hence, we should not use ChunkHolder.level
which might sporadically change as it is used as the underlying storage for tracking the distance to the next chunk ticket. Instead, we should use a version that is better controlled, i.e. only changed in ChunkHolder.tick(...)
as is already provided by ChunkHolder.lastLevel
.
Note that this level should be updated before scheduling the worldgen tasks, so they don't get immediately aborted. Currently, ChunkHolder.lastLevel
gets updated at the end of ChunkHolder.tick(...)
.
Also note that this ChunkHolder.lastLevel
need not be volatile
. It suffices to be atomic with weak memory ordering, as we only require that the observed value from the worldgen thread is indeed sufficiently large if on the main thread since scheduling the task the level never dropped too low. This is automatically provided by java for int
, but of course it does not cause harm to declare the field volatile
(except of negligible performance concerns).
Best,
PhiPro
Linked issues
Comments

Is this still the case in 1.19.2?