MC-177685ChunkTaskPrioritySystem not working as intended
This is more of a code discussion rather than a bug report. Of course, I do not know how it is supposed to work, but I want to show in the following that the ChunkTaskPrioritySystem used in ThreadedAnvilChunkStorage does not add any value and hence does not look working as intended to me. So I think this is worth posting.
In the following discussion I use recent fabric names.
First, I want to give a quick overview over what the instance of ChunkTaskPrioritySystem used in ThreadedAnvilChunkStorage does:
- It manages three dedicated work queues for lighting, worldgen, and things that should run on the main thread
- Each of these dedicated work queues process tasks sequentially in the received order
- The ChunkTaskPrioritySystem receives prioritized tasks and forwards them to the respective dedicated queue
- These priorities are continously updated when a chunk changes its priority (which is basically its distance to the nearest chunk ticket)
- Tasks are forwarded to the dedicated queues according to their priorities
So, the intention behind this class seems to be to schedule the prioritized tasks in the correct order to the dedicated work queues.
However, the problem is that this forwarding is quite fast, usually much cheaper than the forwarded task itself. As a consequence, the tasks get (effectively) instantly forwarded to the sequential queues and lose all their priority information. Then, they might have to wait a long time in those queues, as this is where the real work is happening. So, all the priority information is basically immediately discarded and all tasks just end up in linear queues, basically in the order they werde scheduled.
This does not seem to fullfill the intention above.
There is another instance of ChunkTaskPrioritySystem used in ChunkTicketManager to forward player tickets for chunk loading. This one now indeed throttles the forwarding, so that only a few tickets are actively loading chunks at a time. So, this one (more or less) seems to do its job right.
However, I see the following problem with this:
- The ChunkTaskPrioritySystem in ThreadedAnvilChunkStorage is completely irrelevant for the one in ChunkTicketManager to do its job and could simply be removed
- The throttling done in ChunkTicketManager seems a bit artificial to me. Chunk tickets might be hold back even if they would use uncontended resources (i.e. dedicated work queues that are currently idling). So, this might achieve suboptimal performance.
- The artificial throttling makes the code for ChunkTaskPrioritySystem unnecessarily complicated (although deobfuscation might make things a bit hard to judge here)
Hence, I propose the following simple change:
- Instead of having one ChunkTaskPrioritySystem in ThreadedAnvilChunkStorage that simply forwards tasks to dedicated work queues, have one such ChunkTaskPrioritySystem per dedicated work queue and let it run in the thread context of that queue.
- This way, the ChunkTaskPrioritySystem does not have to forward tasks but can actually carry out tasks itself, as it now runs in the correct thread context. This way, the priority information is not lost.
- More concretely, one might want to keep the underlying sequential dedicated queue for scheduling some tasks directly, rather than via the ChunkTaskPrioritySystem. The concrete change I have in mind is that the ChunkTaskPrioritySystem uses this dedicated queue for its sorting tasks, rather than using a new task queue. This way, the sorting tasks run in the context of the dedicated work queue and are hence allowed to execute tasks directly.
Tasks of the ChunkTaskPrioritySystem are then sequentially sorted with any other tasks directly scheduled to the underlying dedicated queue. (One could now of course give this underlying queue some simpler priorities itself.) - These new instances of ChunkTaskPrioritySystem can now properly detect bottlenecks and carry out tasks in the optimal order.
- The ChunkTaskPrioritySystem in ChunkTicketManager is no longer needed as resources are now already controlled properly and don't need to be throttled further artificially.
This proposal works fine, as long as the bottleneck is single-core-performance, i.e. one of the sequential task queues, and not total-cpu-performance. This is what I would usually expect.
In case total-cpu-performance becomes the bottleneck, one should additionally control the way these dedicated task queues schedule their tasks to the common thread pool. For example, each dedicated queue could schedule its highest priority task (that is continously updated) to the thread pool, which then in turn executes the highest priority tasks until out of resources.
This should now also achieve (quite) optimal results in the case where total-cpu-performance is the bottleneck.
As a nice side effect, this change would allow to simplify the code for ChunkTaskPrioritySystem by quite a bit. Currently, a big source of awkwardness seems to be dedicated to tracking stuff needed for the artificial throttling (and me looking at deobfuscated code ofc 
), for example, such beauties as Either<Function<MessageListener<Unit>, T>, Runnable>. With my proposed changes, these pieces of code would become obsololete and could be removed.
So, in summary:
- In its current form, the ChunkTaskPrioritySystem used in ThreadedAnvilChunkStorage does not provide any value, but only adds quite a bit of overhead.
- My simple proposed changes should achieve better performance and simplify the code quite a bit.
Hope this is helpful 
Best,
PhiPro
Low
NormalCreated Issue:
ChunkTaskPrioritySystem not working as intended
This is more of a code discussion rather than a bug report. Of course, I do not know how it is supposed to work, but I want to show in the following that the ChunkTaskPrioritySystem used in ThreadedAnvilChunkStorage does not add any value and hence does not look working as intended to me. So I think this is worth posting.
In the following discussion I use recent fabric names.First, I want to give a quick overview over what the instance of ChunkTaskPrioritySystem used in ThreadedAnvilChunkStorage does:
- It manages three dedicated work queues for lighting, worldgen, and things that should run on the main thread
- Each of these dedicated work queues process tasks sequentially in the received order
- The ChunkTaskPrioritySystem receives prioritized tasks and forwards them to the respective dedicated queue
- These priorities are continously updated when a chunk changes its priority (which is basically its distance to the nearest chunk ticket)
- Tasks are forwarded to the dedicated queues according to their priorities
So, the intention behind this class seems to be to schedule the prioritized tasks in the correct order to the dedicated work queues.
However, the problem is that this forwarding is quite fast, usually much cheaper than the forwarded task itself. As a consequence, the tasks get (effectively) instantly forwarded to the sequential queues and lose all their priority information. Then, they might have to wait a long time in those queues, as this is where the real work is happening. So, all the priority information is basically immediately discarded and all tasks just end up in linear queues, basically in the order they werde scheduled.
This does not seem to fullfill the intention above.There is another instance of ChunkTaskPrioritySystem used in ChunkTicketManager to forward player tickets for chunk loading. This one now indeed throttles the forwarding, so that only a few tickets are actively loading chunks at a time. So, this one (more or less) seems to do its job right.
However, I see the following problem with this:
- The ChunkTaskPrioritySystem in ThreadedAnvilChunkStorage is completely irrelevant for the one in ChunkTicketManager to do its job and could simply be removed
- The throttling done in ChunkTicketManager seems a bit artificial to me. Chunk tickets might be hold back even if they would use uncontended resources (i.e. dedicated work queues that are currently idling). So, this might achieve suboptimal performance.
- The artificial throttling makes the code for ChunkTaskPrioritySystem unnecessarily complicated (although deobfuscation might make things a bit hard to judge here)
Hence, I propose the following simple change:
- Instead of having one ChunkTaskPrioritySystem in ThreadedAnvilChunkStorage that simply forwards tasks to dedicated work queues, have one such ChunkTaskPrioritySystem per dedicated work queue and let it run in the thread context of that queue.
- This way, the ChunkTaskPrioritySystem does not have to forward tasks but can actually carry out tasks itself, as it now runs in the correct thread context. This way, the priority information is not lost.
- More concretely, one might want to keep the underlying sequential dedicated queue for scheduling some tasks directly, rather than via the ChunkTaskPrioritySystem. The concrete change I have in mind is that the ChunkTaskPrioritySystem uses this dedicated queue for its sorting tasks, rather than using a new task queue. This way, the sorting tasks run in the context of the dedicated work queue and are hence allowed to execute tasks directly.
Tasks of the ChunkTaskPrioritySystem are then sequentially sorted with any other tasks directly scheduled to the underlying dedicated queue. (One could now of course give this underlying queue some simpler priorities itself.)- These new instances of ChunkTaskPrioritySystem can now properly detect bottlenecks and carry out tasks in the optimal order.
- The ChunkTaskPrioritySystem in ChunkTicketManager is no longer needed as resources are now already controlled properly and don't need to be throttled further artificially.
This proposal works fine, as long as the bottleneck is single-core-performance, i.e. one of the sequential task queues, and not total-cpu-performance. This is what I would usually expect.
In case total-cpu-performance becomes the bottleneck, one should additionally control the way these dedicated task queues schedule their tasks to the common thread pool. For example, each dedicated queue could schedule its highest priority task (that is continously updated) to the thread pool, which then in turn executes the highest priority tasks until out of resources.
This should now also achieve (quite) optimal results in the case where total-cpu-performance is the bottleneck.As a nice side effect, this change would allow to simplify the code for ChunkTaskPrioritySystem by quite a bit. Currently, a big source of awkwardness seems to be dedicated to tracking stuff needed for the artificial throttling (and me looking at deobfuscated code ofc
So, in summary:
- In its current form, the ChunkTaskPrioritySystem used in ThreadedAnvilChunkStorage does not provide any value, but only adds quite a bit of overhead.
- My simple proposed changes should achieve better performance and simplify the code quite a bit.
Hope this is helpful
Best,
PhiPro
![[Mod] Michael Wobst](../../../data/avatars/users/37868.png){kind=small}
relates to
![[Mojang] slicedlime](../../../data/assets/gravatar/slicedlime.jpeg){kind=small}
Important![[Helper] Jack McKalling](../../../data/avatars/users/92859.png){kind=small}
relates to
This is more of a theoretical issue and maybe not so relevant for practical purposes. I mainly post this because any bug makes it hard/impossible to properly reason about code and because this issue is automatically fixed by my proposed solution for MC-177685 , so it is mainly intended as a supplement for that one.
The issue can occur when a player chunk ticket gets scheduled for removal and shortly after another player chunk ticket gets scheduled for addition for the same chunk. The problem is that the scheduled removal will cancel some pending additions at the time the removal is executed.
private void updateTicket(long pos, int distance, boolean oldWithinViewDistance, boolean withinViewDistance) { if (oldWithinViewDistance != withinViewDistance) { ChunkTicket<?> chunkTicket = new ChunkTicket(ChunkTicketType.PLAYER, ChunkTicketManager.NEARBY_PLAYER_TICKET_LEVEL, new ChunkPos(pos)); if (withinViewDistance) { ChunkTicketManager.this.playerTicketThrottler.send(ChunkTaskPrioritySystem.createMessage(() -> { ChunkTicketManager.this.mainThreadExecutor.execute(() -> { if (this.isWithinViewDistance(this.getLevel(pos))) { ChunkTicketManager.this.addTicket(pos, chunkTicket); ChunkTicketManager.this.chunkPositions.add(pos); } else { ChunkTicketManager.this.playerTicketThrottlerSorter.send(ChunkTaskPrioritySystem.createSorterMessage(() -> { }, pos, false)); } }); }, pos, () -> { return distance; })); } else { ChunkTicketManager.this.playerTicketThrottlerSorter.send(ChunkTaskPrioritySystem.createSorterMessage(() -> { ChunkTicketManager.this.mainThreadExecutor.execute(() -> { ChunkTicketManager.this.removeTicket(pos, chunkTicket); }); }, pos, true)); } } }
The problematic part is basically
ChunkTicketManager.this.playerTicketThrottlerSorter.send(ChunkTaskPrioritySystem.createSorterMessage(() -> { ... }, pos, true));
which will remove all pending tasks at the same chunk residing in a LevelPrioritizedQueue at the time this task is executed on the ChunkTaskPrioritySystem.
As a result, if a re-addition of a player chunk ticket is scheduled before earlier removals have finished their execution on the ChunkTaskPrioritySystem, this can lead to the re-addition being canceled and hence to a loss of chunk tickets.
As it turns out, this is actually quite hard to achieve. The argument for this, however, requires some deeper knowledge of the internal ordering of ChunkTaskPrioritySystem and is hence in my opinion not really well suited for simple arguments about code correctness. Furthermore, there will be a slight bug in the following argument.
The problematic case occurs when a chunk ticket gets scheduled for addition after a scheduled removal. So, let's consider such a situation.
- When the ChunkTaskPrioritySystem looks at the addition task, it does not directly execute it, but first sorts it into a LevelPrioritizedQueue. Before that point, the task resides in some simpler prioritized queue together with the removal task.
- By the internal priorities of this simple queue, the ChunkTaskPrioritySystem will look at the scheduled removal before it looks at the scheduled addition, because it has a higher priority and was added earlier. (This is the buggy step in the argument!)
- When it looks at the scheduled removal, it will directly execute its associated action (the removal of the chunk ticket) and furthermore remove all pending tasks in the LevelPrioritizedQueue. However, at this point the addition task still resides in the simple queue and not in the LevelPrioritizedQueue, so it will actually not be removed.
- Hence, everything is actually fine and works as it should
Now, as pointed out, the bug lies within the floowing false statement:
If a task T1 gets added to a TaskQueue.Prioritized before a task T2 and it has a strictly higher priority then it gets returned by poll() before task T2.
The problem is that while the individual queues for each priority level are threadsafe, the collection as a whole is not, in the sense described here, i.e. it does not satisfy some basic PRAM consistency (or whatever you like).
So, this makes the above argument invalid and allows to break it with some very precise timing. However, due to this precise timing, it is not really possible to break multiple chunk tickets at once, so there will always be enough tickets around to hide the effect. Hence I cannot really provide concrete steps to visualize the issue.
As stated in the beginning, this is more of a theoretical consideration for arguing about code correctness and a supplement to MC-177685.
The case where a removal gets scheduled after an addition, on the other hand, looks unproblematic, as this case explicitly checks whether the ticket should still be added right before doing so.
As a proposed solution, I think doing a similar check right before the removal of a ticket and removing the code that cancels all pending tasks, should solve the issue.
However, I prefer the solution of removing the ChunkTaskPrioritySystem used in ChunkTicketManager alltogether, as proposed in MC-177685, which will then automatically solve this issue as well.
Best,
PhiPro
Took me quite a while to fit all the puzzle pieces together, but I finally have a satisfying explanation for this bug with all its weird features.
Basically, it is caused by MC-170010 and my proposed fix for it will fix this issue as well.
Let me first explain what is causing the lagspikes. It is basically the same lagspike as experienced in the following issue:
- Create a new redstone-ready world
- /setblock 7 103 7 minecraft:glass
- Experience a clientside lagspike
This lagspike occurs whenever a block is placed high above the ground, where "ground" really means the topmost block of all neighbor chunks. As soon as there is only a single block above, the lag spike disappears. For example, if you /setblock 7 255 7 minecraft:glass then the above steps won't cause any lagspike.
The reason for this is MC-170010. As mentioned there, topmost skylight maps are not properly initialized to their previous values. Instead, they are created completely dark and then get relighted. Placing a block high above ground creates 27 new lightmaps. Due to the order of creating them bottom to top, all of them will be uninitialized and have to be relighted. So, although we place a glass block, which doesn't cause any light change at all, the client has to relight 27 subchunks at once due to this bug, which takes quite some time.
As soon as there is any block above, no (or fewer) lightmaps get created, hence avoiding the issue.
So, my proposed fix for this issue is my proposed fix for MC-170010.
While not really relevant for the solution, I think it is still interesting to explain why the bug doesn't occur in many situations and why it is related to spawnchunks and is also stateful. So, let's start by explaining how the previous lag spike relates to the one of this bug report.
The following easy setup is sufficient for triggering it:
- Create a new redstone-ready world
- Set the render distance to 2 (or 3)
- /setblock 7 103 7 minecraft:glass
- Move exactly 4 chunks in any cardinal direction, e.g. move to chunk (4 0)
- Move 1 chunk back, e.g. to (3 0), and experience a lag spike
When you move back from (4 0) to (3 0), the chunk (0 0) gets loaded on the client. This causes the lightmaps to be created around subchunk (0 6 0) and hence leads to the exact same issue as previously described. Namely, those lightmaps are created uninitialized and have to be relighted all at once, which costs too much time. (Actually, in this case not all of the 27 lightmaps are directly initialized, but only those in the already loaded chunks. Nevertheless, this seems to be enough to lag the client quite bad.)
Why does it happen only for certain chunk borders and not all of the time?
First of all, the issue relies on lightmaps being created upon loading chunks on the client. The lightmaps created for the newly loaded chunk are in fact initialized, namely to the data the client receives from the server. So those don't contribute. (Actually, the topmost lightmap of the newly loaded chunk is still thrown away and relighted in any case. But this single lightmap seems to have sufficiently small impact to not cause lag spikes all of the time. Nevertheless, this unnecessary relighting would be eliminated with my proposed solution as well.) So, a necessary condition for the issue is that the newly loaded chunk is much higher than its already loaded neighbors.
Nevertheless, even in this case, there are still a lot of awkward situations that avoid the bug:
- The bug doesn't occur if you move e.g. to (5 0) and then to (3 0)
- or if you reload the world at (4 0).
- Also, if you replace the block by air and then by glass again in the above steps, the issue doesn't occur either.
- If you move the spawnchunks far away from (0 0) the issue doesn't occur.
These strange stateful features of the bug were really giving me some headaches while hunting it down The block isn't loaded on the client while at (4 0), so replacing it by air and then glass again shouldn't have any effect on the client state, yet it turns out that it avoids the lag spike.
The easiest case is what happens if you reload the world at (4 0). In this case, when the server sends the data for chunk (1 0), it also sends the lightmaps for (1 5 0), (1 6 0) and (1 7 0). Similarly for (1 -1) and (1 1). Those lightmaps are not directly added to the world, since there is no nearby block yet. Instead they are kept queued. Once we move to (3 0), the block at (0 6 0) creates all the surrounding lightmaps. Now, in this case, the lightmaps for (1 -1), (1 0) and (1 1) are not uninitialized, but are initialized from the queued up data that the client received from the server. That is why the bug does not occur in this case.
If we now move back to (4 0), the block at (0 6 0) and hence all the surrounding lightmaps get unloaded. Now, the neighbor chunks do not anymore have any queued up data from the server, and the server won't resend the data unless we move even further away to (5 0). So, when now crossing the chunk border to (3 0) once again, the lightmaps for chunk (1 -1), (1 0) and (1 1) will now indeed be uninitialized and have to be relighted, causing the lagspike.
What happens if you replace the block by air and then glass again while at (4 0)?
When replacing the block by air, the server deletes all the lightmaps. When afterwards spawning the glass block again, the server encounters exactly the same issue of MC-170010 and has to relight the uninitialized lightmaps. This will be detected as ligth changes, although the final light values actually haven't changed. Now, some other piece of code comes into play.
public void flushUpdates(WorldChunk worldChunk) { if (this.blockUpdateCount != 0 || this.skyLightUpdateBits != 0 || this.blockLightUpdateBits != 0) { ... if (this.skyLightUpdateBits != 0 || this.blockLightUpdateBits != 0) { this.sendPacketToPlayersWatching(new LightUpdateS2CPacket(worldChunk.getPos(), this.lightingProvider, this.skyLightUpdateBits & ~this.lightSentWithBlocksBits, this.blockLightUpdateBits & ~this.lightSentWithBlocksBits), true);
What it does is to sync all light updates at the edge of the client's view area. Hence, the (wrongly) detected light updates cause the server to sync the lightmaps for (1 -1), (1 0), (1 1) to the client again, which basically puts us in the same starting point as in the previous case. Those synced lightmaps are used for initialization and hence the issue doesn't occur.
Finally, let us understand the role of spawn chunks. As it turns out, this is due to yet another effect, namely that the chunk tracking distance is 1 larger than the chunk loading distance. Concretely, this means the following:
- The chunk at (0 0) is unloaded from the client upon entering chunk (4 0) (the chunk tracking distance is capped at >= 3)
- Since the chunk loading distance by players is basically one less, i.e. 2, the chunk at (1 0) is already "unloaded" serverside at that time, but still loaded clientside. "Unloaded" means that it is demoted to a border chunk, so it is still loaded in some sense, but it is non-ticking and won't be sent to clients; however, it will be kept loaded on the client if already present.
- When moving to (3 0) the chunk at (1 0) becomes "loaded" again on the server, i.e. it is promoted to a ticking chunk.
- At this point, it will be resent to the client.
- The chunk at (0 0) will only be sent to the client upon moving to (2 0).
- However, as before, the client now has queued light maps, so the lag spike does not occur.
So the difference between spawn chunks and non-spawn chunks is that they are kept loaded on the server and hence shade this discrepancy between tracking and loading distance. This also means that the same rules apply to chunks loaded by any other means, e.g. other players.
Further remarks
As another suggestion, it might be useful to run the client lighting engine offthread as well, as is already done for the server. Of course, you want some way for the client thread to wait for light updates to finish before rendering, as even the slightest latency of light updates near the player usually feels very annoying. Such a way to wait for light updates to finish is needed anyway for MC-164281. Additionally, you probably also want to implement a fix for MC-177685 such that updates near the player can be prioritized and the client thread only has to wait for those, rather than all pending updates. Nevertheless, even without a fix for MC-177685, putting the clientside lighting engine offthread can help mitigating this issue further.
As a final remark, I really strongly recommend to implement my proposed solution for MC-170010 instead of putting more and more band-aid on it, as it makes it increasingly difficult to track down issues. This one already gave me a lot of headaches as it had so many strange fatures related to different code paths. (Admittedly, the challenge was also kindof fun )
Workaround for players
As a temporary workaround for player, you can place a ceiling of minecraft:barrier at y=255 above the whole map. As can be seen from the analysis, the issue arises from a height difference of neighbor chunks. Putting a ceiling above the world eliminates any height difference and reduces the issue to its minimal intensity of flat worlds. Putting the ceiling only above a small part of the world will simply push out the problematic chunk boundaries to the end of the ceiling, so for this workaround to work you need to cover all of the area you visit. While this is definitely not perfect, it should at least help in the case when you are moving in already generated chunks and not visiting new ones.
Best,
PhiPro
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
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
As an additional remark, in order to remove the artificial throttling described above, one should first fix MC-183841, so this doesn't result in more tasks getting executed than before.
On the other hand, even without a fix for MC-183841, a fix for this issue will increase chunk loading performance. since the tasks will be properly prioritized. Nevertheless, without a fix for MC-183841 this might have a negative impact on the total CPU and IO load.
Looks like I somehow managed to overlook some important piece of code... The ChunkTaskPrioritySystem actually does wait for tasks to complete on the dedicated queues before scheduling new ones, instead of scheduling everything at once. So the issue is invalid. Sorry about that.
Nevertheless, I propose the solution explained in the report as a simpler, more efficient and less obscure implementation. So I leave the report open for now.