Uploaded image for project: 'Minecraft: Java Edition'
  1. Minecraft: Java Edition
  2. MC-177685

ChunkTaskPrioritySystem not working as intended

XMLWordPrintable

    • Icon: Bug Bug
    • Resolution: Unresolved
    • None
    • 20w14a, 1.16.3
    • None
    • Community Consensus
    • Chunk loading
    • Normal
    • Platform

      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

            Unassigned Unassigned
            PhiPro Philipp Provenzano
            Votes:
            15 Vote for this issue
            Watchers:
            7 Start watching this issue

              Created:
              Updated:
              CHK: