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

Multiple hopper performance issues

    XMLWordPrintable

    Details

    • Type: Bug
    • Status: Reopened
    • Resolution: Unresolved
    • Affects Version/s: Minecraft 1.8.9, Minecraft 16w02a, Minecraft 1.9, Minecraft 1.10.2, Minecraft 16w43a, Minecraft 1.12.2, 1.15.2, 1.16.1, 20w29a, 20w30a, 1.16.2 Pre-release 1, 1.16.2 Release Candidate 2, 1.16.2, 20w49a
    • Fix Version/s: None
    • Labels:
      None
    • Confirmation Status:
      Confirmed
    • Category:
      Performance
    • Mojang Priority:
      Normal

      Description

      I run a decent sized Minecraft server that tries to not restrict players as much as possible.

      My players love to build redstone contraptions, including item sorters and movers, resulting in LOTS of Hoppers, sometimes hitting 40,000+ Tile Entities, or 2000 Hoppers in a single area.

      This has drastic impact to server performance causing every players experience to be degraded by loss of TPS.

      After EXTENSIVE research using a profiler and studying the code, I've identified numerous issues with the hopper system.

      1. Hoppers by proper design, call the .update() method on an inventory when it has been modified in some way, to trigger physics and comparators.
        However, many inventory manipulation methods also call .update()
        This results in .update being called many times, which then triggers comparators and other physics events. This single handedly crushes servers with hoppers.
      2. Unnecessary ItemStack cloning
        The server calls itemstack.clone() on EVERY item stack it 'trys'. Take TileEntityHopper.a(IHopper) for example.
        If an inventory of say 50 items in a chest is trying to be moved to a hopper, and that hopper only has 1 free slot and nothing matches the chests inventory, this method will result in every one of those item stacks being cloned every tick, only for it to fail to be moved and now an instance of the itemstack will be GC'd
      3. Inefficient Item Suck In behavior
        Hoppers will out-number ItemStacks in any matured world, and has potential to be loaded longer term than an item stack. It does not make sense that hoppers perform AABB lookups for Entities.
        As of 1.13-1.15, this now also does 2 lookups per hopper due to the advanced hitbox. This needs to be a simple lookup again.
        Also, no reason to suck in if there is a full solid occluding block above the hopper.
      4. Usage of streams hurts performance due to overhead, complex call stacks. JVM does better at optimizing non stream code, and Java 8 is not receiving stream optimizations.

      Solutions:
      First I will reference my patch on how I quick patched in solutions, though not always perfect - note there are some non vanilla matters here but will lay out the vanilla relevant elements:
      https://github.com/PaperMC/Paper/blob/10502558e92f478e7e1343193dd8ace8dbf8ac78/Spigot-Server-Patches/0414-Optimize-Hoppers.patch

      1. For cloning issue, I suggest using itemstack count manipulation like I am doing in my patch. This avoids calls to .update() from split stack methods which partially resolves issue #1 also. CraftBukkit made the cloning problem even worse, but Vanilla does pre clone too, so the concept is valid in Vanilla
      2. For the .update call, if switching off using the manipulation methods, some of the .update() calls are now handled, but in TileEntityHopper.c(IInventory, ItemStack, int, EnumDirection) method where the actual inventory manipulation takes place, iinventory.setItem() where itemstack1 is null can also trigger .update() on chests. adding some method to setWithoutUpdate instead on the interface is possibly the most ideal instead of my hack method.
      3. For Hopper Suck In, Instead, ItemStacks could instead look for hoppers to ping them to suck them in, which can do much more efficient Tile Entity BlockPosition lookups. But would still need an Entity AABB for the Minecarts, but the overall occurrence would drastically be reduced.
      4. Hoppers are able to engage the automatic loot filling on a chest. However no reason to run that check multiple times, just do it on first index only
      5. Hoppers in 1.13+ (or at least 1.15) are now sucking in twice, once for each part of the complex shape of a Hopper. Bowl level pixel perfect bounding boxes are unnecessary. Restore the single AABB lookup from 1.12
      6. Removal of all usages of Streams. hoppers are hot. Streams have overhead that added up quite a lot and also seems to hurt JVM optimizations. Since MC runs on java 8, which is not receiving performance improvements to Streams/JIT, this hurts. This includes isEmpty() in Lootable.
      7. Skip item suck in behavior if the block above the hopper is full cuboid solid at all. Items collide with blocks and this scenario should never naturally occur where an item phases into a solid block and then gets sucked in. ( https://github.com/PaperMC/Paper/blob/10502558e92f478e7e1343193dd8ace8dbf8ac78/Spigot-Server-Patches/0414-Optimize-Hoppers.patch#L453 )
      8. Since .getItem() on some inventories (Lootable types) have additional overhead, keep result from first call and pass it to dependent methods

        Attachments

          Activity

            People

            Assignee:
            Unassigned Unassigned
            Reporter:
            aikar Aikar
            Votes:
            32 Vote for this issue
            Watchers:
            16 Start watching this issue

              Dates

              Created:
              Updated:
              CHK: