EssentialsX

EssentialsX

2M Downloads

EssentialsX saves the configuration of players to disk in the main thread causing performance problems.

BoomEaro opened this issue ยท 9 comments

commented

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

  1. Turn on /timings on
  2. Try to die 15 times in a row such as the /suicide command.
  3. Look at the timings. (/timings paste)
  4. 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.)

commented

Having a similar problem too. Much appreciation if this gets patched.

commented

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.

commented

You seem to have missed a crucial part:

private Future<?> delayedSave(final File file) {
if (file == null) {
throw new IllegalArgumentException("File cannot be null");
}
final String data = saveToString();
if (data.length() == 0) {
return null;
}
pendingDiskWrites.incrementAndGet();
return EXECUTOR_SERVICE.submit(new WriteRunner(configFile, data, pendingDiskWrites));
}

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:

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.

commented

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.

commented

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?

commented

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.

commented

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.

commented

With configurate, we now serialize + save off the main thread (save for conversions which need to be blocking on startup)

commented

Just want to mention that the only issue is that saveToString is called on the main thread, and not the actual saving to disk.
This can possibly be fixed by making WriterRunner to execute this instead (only the one that saves the contents).