SignBreak event triggered on physics update (optimize?)
NullCase opened this issue · 9 comments
This has been a thing for a while. Just getting around to it now.
Plugin Version
3.9.3-SNAPSHOT (build 117)
Plugin Config
Server Version
Paper version git-Paper-597 (MC: 1.13.2) (Implementing API version 1.13.2-R0.1-SNAPSHOT)
Server Log
In the Nether
ChestShop::Event: c.A.C.L.B.B.SignBreak (BlockPhysicsEvent)count(4789926) total(1.53% 1.545s, 2.29% of tick)avg(0.00ms per - 1.15ms/3,556.00 per tick)
WorldGuard::Event: c.s.w.b.l.WorldGuardBlockListener (BlockPhysicsEvent)count(4789926) total(1.44% 1.448s, 2.15% of tick)avg(0.00ms per - 1.08ms/3,556.00 per tick)
In Overworld
ChestShop::Event: c.A.C.L.B.B.SignBreak (BlockPhysicsEvent)count(1808519) total(0.45% 0.456s, 0.68% of tick)avg(0.00ms per - 0.34ms/1,342.63 per tick)
WorldGuard::Event: c.s.w.b.l.WorldGuardBlockListener (BlockPhysicsEvent)count(1808519) total(0.44% 0.444s, 0.66% of tick)avg(0.00ms per - 0.33ms/1,342.63 per tick)
What other plugins are you running?
https://timings.aikar.co/?id=0e76492ae9074e55af1693be98d03952
What is happening?
Sign break event being triggered while remove shops/chests = false
REMOVE_EMPTY_SHOPS: false
#If true, if the shop is empty, the sign is destroyed and put into the chest, so the shop isn't usable anymore.
REMOVE_EMPTY_CHESTS: false
#If true, if the REMOVE_EMPTY_SHOPS option is turned on, the chest is also destroyed.
What did you expect to happen?
I don't know. It seems like ChestShops are constantly checking to see if REMOVE_EMPTY_SHOPS: true. I include WorldGuard here because the event count is exactly the same. So, one causes the other or they're both caused by something else... or it's an incredible coincidence.
This listener has nothing to do with these config options. The plugin basically has to check if a block that gets updated by a physics event is a shop sign that gets destroyed in order to send the destroy event. See the listener method. (I'm a bit sceptical if that event is even called for signs though)
If it really is called for that then I'm not sure what else could be done in that case (besides maybe exiting early if the physics event is in a disabled world or adding an option to completely disable the calling of the destroy event on physics events) I guess just avoiding massive amounts of physics updates would also get rid of the performance impact.
Avoid physics updates? That sounds like when you tell the Dr. "It hurt's to think." And he says "well just stop doing it." I don't know how that will go xD
I'm sorry, idk how to code. That's why I'm happy to pay smart people given the option.
How much would it cost to address this? Because it sounds like this tiny amount of lag which occurs so often will have a big computational cost in the long run. For myself and everyone using ChestShop.
I looked into that a bit more and it's not possible to completely remove listening on the event without breaking functionality in weird ways (at least not on Bukkit/Spigot). Besides checking for gravity blocks that would destroy shops it also is essential for properly handling the case where the block that the sign is attached to gets broken so if a config option to make that optional was added then that would break functionality unexpectedly.
The listener itself already exits as early as possible when it detects that the block updating is not a sign so I don't think anything can be optimised there. (This could be verified by running a profiler though) One possible solution exists though but that would require an event from the Paper-API (BlockDestroyEvent) and I would like to keep compatibility with Bukkit so we would need to figure out a good way of doing that.
OK, it sounds like this particular route (watching for physics updates) to solve the "is there still a shop here?" question is as optimized as it can be.
And that Paper-API has an alternative route, but this is not available in Bukkit which would cause problems.
Yes, basically. I've had an idea how to work around this issue without having to add special compatibility for Paper in ChestShop (for now) by using a helper plugin that replaces BlockPhysicsEvent listeners with BlockDestroyEvent ones. I just finished implementing this so if you want to test it out already feel free to do so.
If you only want it to do the replacing for ChestShop where I'm almost 100% sure it will not cause issues then add this to the plugin config:
plugins:
- ChestShop
WOW.
OK, it's early and i'm still testing. Here's what i've noticed so far.
- ChestShop
- CreativeGates
- WorldGuard
In all three cases, i'm getting a 50-90% savings on computation for piston activity.
Test:
operate about 20 pistons constantly, moving around blocks. test with/without plugins added to list.
Result:
Before adding CreativeGates and WorldGuard
https://timings.aikar.co/?id=f61a04809b6a4596a5d5daf25efb1b79
After adding both.
https://timings.aikar.co/?id=07f895f7b63149beb1c21d55fde00cb9
edit: Our live server's next update is in ~a week. For now it's running on our test world. So I'll come back in a week with something else.
Just fyi regarding both CreativeGates and WorldGuard: Replacing the BlockPhysicsEvent with my plugin will most likely break some functionality in them. In WorldGuard the flag to stop blocks from falling will stop to work and in CreativeGates portal blocks might disappear. (Although I'm not too sure about that one)