Optimize Hoppers - Closes #392 #976

aikar · 2018-01-18T06:02:13Z

Optimize Hoppers

Lots of itemstack cloning removed. Only clone if the item is actually moved
Return true when a plugin cancels inventory move item event instead of false, as false causes pulls to cycle through all items.
However, pushes do not exhibit the same behavior, so this is not something plugins could of been relying on.
Add option (Default on) to cooldown hoppers when they fail to move an item due to full inventory
Skip subsequent InventoryMoveItemEvents if a plugin does not use the item after first event fire for an iteration

Closes #392

aikar · 2018-01-18T06:37:26Z

Spigot-Server-Patches/0264-Optimize-Hoppers-with-Plugin-Heuristics.patch

+                } else if (ihopper instanceof EntityMinecartHopper) {
+                    ((EntityMinecartHopper) ihopper).setCooldown(ihopper.getWorld().spigotConfig.hopperTransfer / 2);
+                }
+                return false;


note to self, change this to true. false causes this code to be ran for every item. but because we are mode 2, we know item is not going to change the fact this event was cancelled.

return true in mode 0 too. i was originally concerned it would block plugins who want to cancel first and allow 2nd, but the same cant be said for push mode, so we can just make them consistent, and greatly improve performance of cancelled pulls.

UberMC · 2018-01-19T19:58:51Z

This looks interesting!

aikar · 2018-01-20T02:35:11Z

Exciting, found ways to do these optimizations always without any mode setting, greatly reducing diff and should keep things super safe with plugins.

aikar · 2018-01-20T02:47:51Z

I have uploaded a paperclip patcher for this patch to <outdated, see Z's link below>

I would love to get feedback from users with bad hopper timings who use protection plugins or other hopper related plugins.

I'm more interested in knowing that behavior isn't broken than I am about performance, but if you can get a before and after timings/profiler results, that would be good too.

RoboMWM · 2018-01-20T03:27:36Z

I personally would like a mode 0 or 1 toggle at the very least so I can just ignore/effectively eliminate the event regardless if plugins use it but I guess the best way to go about that is to do something similar to eliminating use of PlayerChatEvent.

aikar · 2018-01-20T03:30:22Z

@RoboMWM There was a config added for that.
https://github.com/PaperMC/Paper/pull/976/files#diff-2818bedee6c292733614b57195f52435R25

aikar · 2018-01-25T02:35:52Z

Updated patch to be on top of the latest Paper as of today (with my Cat Hopper Performance fix too)

Updated configs to be under the existing hopper. section

fixed bug with suck in code not returning false for item not found, and also fixed it to respect the cooldown if full setting.

This should be good to go now, need testers.

NullCase · 2018-01-29T04:57:59Z

Re: hoppers picking up dropped items...

Items still enter blocked hoppers given hoppers: Push-based true

mibby · 2018-02-05T03:50:07Z

@aikar Is there any easy way to apply your patch on top of the latest paper jar? I tried compiling upstream with your PR merged but am running into issues with the decompile script during compilation.

I ran a timings profile recently and noticed the majority of my tick impact was coming from InventoryMoveItemEvent by two plugins (WorldGuard / ChestShop). I could probably be used as a helpful test candidate to see how much improvement is made with your optimization patch.
https://timings.aikar.co/?id=c6ccbd224d894c3690de6e78aac5d082#timings

zachbr · 2018-02-05T04:09:20Z

This patch applied to latest (1322)
https://zachbr.keybase.pub/paper/github/pulls/976/paperclip.jar

mibby · 2018-02-06T16:00:05Z

So far early timing indication shows a massive improvement to hopper performance in my environment. I do not notice InventoryMoveItemEvent in the timings profile whatsoever. I will report back later today with a new timings report during around peak hour at the same time as the earlier report.

    hopper:
      cooldown-when-full: true
      disable-move-event: false
      push-based: false

aikar · 2018-02-06T18:13:06Z

@mibby behavior wise, does things still function as expected? IE WorldGuard / chestshop able to block restricted hopper access, etc?

If so, then we will be good to merge this!

mibby · 2018-02-07T03:27:02Z

@aikar I believe so. I created a ChestShop shop for testing and couldn't move items out via a hopper. Protecting a chest via LWC, couldn't move items out via hopper unless flagged to do so. Hoppers can move items out of normal chests like vanilla behavior just fine.

Unsure how to test hoppers with WorldGuard since there is no protection flag for it. I assume it is also working fine as intended since the above does.

New timings with no noticeable impact from InventoryMoveItemEvent. However I doubt the same problem chunks that initially caused the load are presently loaded in this timings, but at first glance, greatly improved performance.
https://timings.aikar.co/?id=7620516f970a456c9b48c27f9bb7fae5#timings

ArtNRG · 2018-02-09T10:30:11Z

Items can't get into the hopper through a soil with this optimization.

push-based: true
cooldown-when-full: true
disable-move-event: true

aikar · 2018-02-09T18:42:08Z

@ArtNRG what do you mean through a soil? This change doesn't alter hopper suck in mechanics, it impacts the Bukkit Event.

Try with push-based disabled, as that is not related to this PR.

- Lots of itemstack cloning removed. Only clone if the item is actually moved - Return true when a plugin cancels inventory move item event instead of false, as false causes pulls to cycle through all items. However, pushes do not exhibit the same behavior, so this is not something plugins could of been relying on. - Add option (Default on) to cooldown hoppers when they fail to move an item due to full inventory - Skip subsequent InventoryMoveItemEvents if a plugin does not use the item after first event fire for an iteration

aikar self-assigned this Jan 18, 2018

This was referenced Jan 18, 2018

InventoryMoveItemEvent improvements #445

Closed

Hopper Performance - Research and Suggestions #392

Closed

aikar commented Jan 18, 2018

View reviewed changes

aikar force-pushed the optimize-hoppers branch from 45a51e5 to d8597ab Compare January 19, 2018 04:05

aikar force-pushed the optimize-hoppers branch from d8597ab to 3e735af Compare January 20, 2018 02:33

aikar changed the title ~~Optimize Hoppers with Plugin Heuristics~~ Optimize Hoppers Jan 20, 2018

aikar force-pushed the optimize-hoppers branch from 3e735af to aabc698 Compare January 20, 2018 03:01

aikar force-pushed the optimize-hoppers branch from aabc698 to 6b5ee41 Compare January 25, 2018 01:10

aikar mentioned this pull request Jan 27, 2018

Hoppers picking up dropped items even when a chest is above the hopper. #993

Closed

Phoenix616 mentioned this pull request Feb 5, 2018

Performance impact from ChestShop::Event: c.A.C.L.I.ItemMoveListener (InventoryMoveItemEvent) ChestShop-authors/ChestShop-3#109

Closed

aikar changed the title ~~Optimize Hoppers~~ Optimize Hoppers - Closes #392 Feb 6, 2018

aikar force-pushed the optimize-hoppers branch from 6b5ee41 to cb2d5bb Compare February 13, 2018 04:25

aikar force-pushed the optimize-hoppers branch from cb2d5bb to 094bb03 Compare February 13, 2018 04:26

aikar merged commit 094bb03 into master Feb 13, 2018

aikar deleted the optimize-hoppers branch March 7, 2018 16:03

paul-palmer mentioned this pull request Jul 14, 2022

Hoppers into Composters and InventoryMoveItemEvent #8142

Closed

Uh oh!

Optimize Hoppers - Closes #392 #976

Optimize Hoppers - Closes #392 #976

Uh oh!

Conversation

aikar commented Jan 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aikar Jan 18, 2018

Choose a reason for hiding this comment

Uh oh!

UberMC commented Jan 19, 2018

Uh oh!

aikar commented Jan 20, 2018

Uh oh!

aikar commented Jan 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RoboMWM commented Jan 20, 2018

Uh oh!

aikar commented Jan 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aikar commented Jan 25, 2018

Uh oh!

NullCase commented Jan 29, 2018

Uh oh!

mibby commented Feb 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zachbr commented Feb 5, 2018

Uh oh!

mibby commented Feb 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aikar commented Feb 6, 2018

Uh oh!

mibby commented Feb 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ArtNRG commented Feb 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aikar commented Feb 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

aikar commented Jan 18, 2018 •

edited

Loading

aikar commented Jan 20, 2018 •

edited

Loading

aikar commented Jan 20, 2018 •

edited

Loading

mibby commented Feb 5, 2018 •

edited

Loading

mibby commented Feb 6, 2018 •

edited

Loading

mibby commented Feb 7, 2018 •

edited

Loading

ArtNRG commented Feb 9, 2018 •

edited

Loading