EssentialsX saves the configuration of players to disk in the main thread causing performance problems.
BoomEaro opened this issue ยท 9 comments
Information
Full output of /ess version
:
[14:58:03] [Server thread/INFO]: Server version: 1.12.2-R0.1-SNAPSHOT git-Spigot-642f6d2-6103339 (MC: 1.12.2)
[14:58:03] [Server thread/INFO]: EssentialsX version: 2.15.0.45
[14:58:03] [Server thread/INFO]: LuckPerms version: 4.3.2
[14:58:03] [Server thread/INFO]: Vault version: 1.5.6-b49
[14:58:03] [Server thread/INFO]: EssentialsXChat version: 2.15.0.45
[14:58:03] [Server thread/INFO]: EssentialsXSpawn version: 2.15.0.45
Details
Description
EssentialsX each time a player dies or when the /sethome command is executed, overwrites the player's config in the main thread causing performance problems.
Steps to reproduce
- Turn on /timings on
- Try to die 15 times in a row such as the /suicide command.
- Look at the timings. (/timings paste)
- In the timings there will be information that there is a problem with the Essentials. It is also noticeable that at each death, the player's configuration file is immediately updated.
Expected behavior
The absence of lags in death caused by the recording of the config in the main thread.
I think it is worth creating a buffer in which to write information about whether the config has changed.
And then just create a timer that will be in another thread to save the changes to disk.
(I may be mistaken in the logic of saving players, but after looking at the plugin code, I came to this conclusion. Maybe everything works a little differently.)
I don't know what "Not enough info" means. Well, okay, I'll show you in more detail.
For the experiment, take Vault supporting Essentials economics.
If you call the method "Economy#withdrawPlayer", the time spent on writing off money will be significantly high.
Here is the code that I tested: https://pastebin.com/rhF3HsxV
The result is as follows:
[15:37:12] [Server thread/INFO]: time 3035751 ns
That is a whole 3 milliseconds, which is too much in the main thread.
Next, I traced what really happens.
Here is a class from Vault: https://github.com/MilkBowl/Vault/blob/master/src/net/milkbowl/vault/economy/plugins/Economy_Essentials.java
As you can see, when withdrawing funds, this operation is invoked by Essentials. Next, look what's there:
https://github.com/EssentialsX/Essentials/blob/2.x/Essentials/src/com/earth2me/essentials/api/Economy.java
Next, find out exactly how Essentials sets the money:
https://github.com/EssentialsX/Essentials/blob/2.x/Essentials/src/com/earth2me/essentials/UserData.java
As you can see, often on each method of installing a new configuration there is a config.save();
Next, we see that there is a stopTransaction() method.
We go to the parent EssentialsConf:
https://github.com/EssentialsX/Essentials/blob/2.x/Essentials/src/com/earth2me/essentials/EssentialsConf.java
And here we see the full picture: https://pastebin.com/NU0UwqyC
As a result, it turns out that EVERY use can cause the server to lag. And not only, in the UserData class every action, like /sethome /kit is written to the config right away in the main thread. This is a big performance problem which is worth fixing.
I propose to rework the save() method in such a way that all the savings would be in another thread. This will fix all performance problems. For example, you can use bukkit asynchronous scheduler.
You seem to have missed a crucial part:
Essentials/Essentials/src/com/earth2me/essentials/EssentialsConf.java
Lines 260 to 274 in a1297fe
EssentialsX should always write to disk in a separate thread. As far as I can see there is no instance whatsoever where writing to disk occurs on the main thread.
There are some very selective cases where EssentialsX waits on a thread for the write to complete, using the forceSave
method. These cases are:
- Creating a new economy NPC's user data to ensure NPCs are saved correctly (depends on other plugins, typically async)
- Performing EssentialsUpgrade data migrations to ensure migrated data is saved (always happens during server startup so never impacts gameplay)
- Deleting user data (must be initiated manually through commands)
- Saving user data on logout which is always called asynchronously and therefore never occurs on the main thread
In every other case, writing to disk does not occur on the main thread.
I may have determined the cause of the delay, but I'll need to investigate this further before I can conclude what the exact issue is. What I can say for certain is that writing to disk is not responsible for these delays.
Update: as I suspected, the culprit is the YAML serialization. I'm not convinced there is a safe way around this - even if the userdata was serialized on a different thread, it would require being synchronized which still has the potential to lock the main thread if several modifications happen within the same tick.
After I wrote the message, I continued to study the problem further and yes, at the very end, like you, I came to the conclusion with the help of the Spark plugin that the YAML serialization takes some time.
I had the basic idea, to do something like a timer, which once at a certain time, would check all loaded users for changes (boolean true, if there were any changes with this object). And then in the same stream where the timer is running, save it to disk if changes == true. WorldGuard has about the same thing, though as far as I understand it, it causes the task of saving after some time after some change, and everything is saved there all at once and not separately, but perhaps this is not so. (I only guess, because the source did not look)
Does it make sense to do such a timer or maybe there are solutions for better?
The timer as it performs several functions in itself. During the iteration over the loaded players, the players that need to be saved were also saved there, and the YAML serialization worked there too. That is, the timer will just read the information from the objects of users with UserMap, and the resulting information will be written to disk. I do not know how you can implement it without breaking anything.
I don't think it's valid to save every user on a timer. This alone doesn't solve the problem that serialisation is slow. It also means data won't be saved as soon as possible, which may impact plugins like the MySQL plugin (though we don't officially support that plugin).
Currently, each EssentialsConf
instance has its own queue for write tasks to be run on a separate thread. When there are multiple tasks queued, only the last task actually writes to disk while the other ones exit. This avoids multiple out-of-date writes to disk. However, the write task doesn't include the serialisation - this always happens on the original thread and is never skipped.
I did try moving serialisation into the write task, but this seemed to cause a deadlock, so I'd need to look into it further to see what's causing it.
With configurate, we now serialize + save off the main thread (save for conversions which need to be blocking on startup)