Shopkeepers

Shopkeepers

2M Downloads

16% of server tick for ONE shopkeeper

CeIebrimbor opened this issue ยท 8 comments

commented

Preliminaries:

  • Shopkeepers version: 2.7.1
  • Spigot version: git-Paper-76 (MC: 1.14.2) (Implementing API version 1.14.2-R0.1-SNAPSHOT)

  • I have checked that my issue/question does not get answered by:

The FAQ:

  • The FAQ addresses tick consumption during world save, but the recommendation suggests reducing the number of Shopkeepers to alleviate the load. I only have one shopkeeper in the entire server.

Reproduction on a fresh and up-to-date Spigot server:

  • Use 1.14.2 server and wait for World Save to occur. As everyone knows, more players on 1.14.2 equates to a lot of load, so a single player loading a small amount of chunks might not result in the same load.
  • This load I experienced happens when 3-4 players are online.
  • Spigot 1.14.2 has crippling lag during World Save, so the unusual load from Shopkeepers is buried.
  • Paper (aka PaperSpigot) has a slower, incremental world saving that does not lag the server during the save event. With World Saving lag removed entirely, the load from Shopkeepers during the save is 10x the load of the world save itself. As seen below.

The issue:

image

image

Note: I have tried the following settings and there was no change...
use-legacy-mob-behavior: true
save-instantly: false

commented

Using the latest build of Spigot I was able to reproduce around 5% (2.5ms) with 2 online players. For comparison, I observed timings of 927 - 5686 ms for the saving itself.

I made some changes, which reduced it to around 1% in my testings (with 3 chunks containing shopkeepers).

However:

Paper (aka PaperSpigot) has a slower, incremental world saving that does not lag the server during the save event

To prevent shopkeeper entities and blocks to get persisted with the world/chunk data (so that all shopkeepers are gone if you decide to no longer use the plugin, or in case of server crashes), the plugin removes all shopkeeper entities and blocks from the world prior to world save and respawns them again 1 tick afterwards.
I haven't looked into Paper's implementation yet, but Shopkeepers requires the world save to be completed the tick after the WorldSaveEvent (when it reloads the Shopkeeper blocks and entities) to function properly. If this feature of Paper breaks this assumption, then you might encounter the issue of duplicated entities on server crashes or shopkeeper entities and blocks still existing after no longer using the plugin..
Edit: This issue might actually only affect sign shopkeepers.

commented

I haven't looked into Paper's implementation yet

Paper's incremental chunk saving is relatively new. See commit here:
PaperMC/Paper@6f2c8a6

This issue might actually only affect sign shopkeepers.

I don't use Sign shopkeeper. The one keeper I have is a Citizens Admin Shop.

Here is my tick consumption for my heaviest plugins:
image

commented

Okay, if this calls another WorldSaveEvent for every of its incremental saves then this should not be an issue.

commented

I overlooked something while rewiring an old patch for 1.14, should be fixed on our side now (b77+) (Well, the firing of events all the time), this is a patch that we've had for a good while on our side for a few versions

commented

As for your actual system, that's going to be broken, I really don't like the idea of adding such an event, but ChunkSaveEvent, maybe? for entities themselves you should be able to set their persistence using the API, blocks is a fun one, which would require something to know what chunk is actually being saved right now

commented

I overlooked something while rewiring an old patch for 1.14, should be fixed on our side now (b77+) (Well, the firing of events all the time), this is a patch that we've had for a good while on our side for a few versions

So firing the WorldSaveEvent for the individual (incremental) world saves was not actually intended?

ChunkSaveEvent, maybe?

Well, this would indeed be nice to have in case these incremental saves were an official minecraft / spigot feature. But I am trying to avoid any Paper-specific special cases / use of Paper-specific API ..
Any other plugins not aware of this Paper-specific behavior might break as well if they rely on the default minecraft / Spigot world saving and WorldSaveEvent behavior (though I admit that my usecase might be rather specific, since most plugins probably don't care about performing cleanup on world saves..).

The timings reports in the associated issue mentioned 8 WorldSaveEvents per tick. So if the issue is that calling the WorldSaveEvent for plugins introduces plugin-side overhead, what about a tradeoff between the occasional full saves of vanilla minecraft and the high-frequency incremental saves by Paper?

commented

@CeIebrimbor
Updating Paper should solve the main issue, but a snapshot which should further improve the performance of handling world saves is available here: https://nexus.lichtspiele.org/repository/snapshots/com/nisovin/shopkeepers/Shopkeepers/2.7.2-SNAPSHOT/Shopkeepers-2.7.2-20190610.221926-2.jar

commented

Firing it every tick was not intentional, given the fact that plugins do use that event for saving data and other expensive events they deal with, it would cause the very issue that you see here where a plugin is doing things in that event, but, your usage of the event is very, something I've never seen before :/

The issue with doing full saves is that it puts us back into the lag bomb that auto-save causes, especially in 1.14, that's super harsh on the server itself