Lag from a simple EventHandler

Discussion in 'Spigot Plugin Development' started by Roree, Apr 23, 2017.

  1. I have been told that the ComamndPreprocessEvent listener in my EssentialsHook class has been causing lots of lag until the server crashed. I'm not sure why it would be causing that much lag when the only time the listener runs more than 4 lines of code is when the command starts with /ban, /tempban or /banip. Even when the command is a ban tempban or a banip it simple loops through json values 1 tick later to see if the player was actually banned or not AND the player has to be frozen for the code to even loop through the JSON file. Whats also strange is they told me that they never frozen anyone in the time the server was up (meaning the isBanned method which loops through JSON files would never have ran in the first place so that can't be causing the lag)

    Timings:
    How could my code ever take 244ms anyways?

    Here is the class:
    https://github.com/7rory768/SimpleF...ns/simplefreeze/hooks/EssentialsHook.java#L38
     
    #1 Roree, Apr 23, 2017
    Last edited: Apr 24, 2017
  2. sothatsit

    Patron

    Bukkit.getOfflinePlayer has to find the players UUID from their name and is as such incredibly slow. This should be done asynchronously or not at all.
     
  3. Either way it shouldn't be causing that much lag for every single command unless it's ban, tempban or banip.
     
  4. Still need help with this. Maybe someone can run SimpleFreeze on their server for a few hours so I can get an accurate timing report?
     
  5. Do you know what a "blocking operation" is? And why it freezes the thread it is running on?
     
  6. sothatsit

    Patron

    Remove the getOfflinePlayer call or make it asynchronous and then see if it is still causing lag. My guess would be 100% not.
     
  7. Nope

    I never learned what asynchronous means in school, is there somewhere I can read about it?
     
  8. 1; Method/function calls dont just happen instantly. A blocking call is something like reading from a stream (by extension, reading from console), a request to an external website, a database lookup - the operation takes a good deal of time to complete (either waiting for input or a reply), and since the thread cannot continue until the method returns, the method "blocks" the thread until it has something to return (or simply freezes the program if the operation never completes and does not have a time limit).

    2; Google. It basically means un-synchronized, and in programming it refers to doing multiple tasks at once, that are not connected and run at their own speed. Typically anything that is going to pause the server for more than a few milliseconds should be done on another thread instead, like looking up offline players, database requests, ect.

    Various kinds of designs such as callback (make a new thread, do something, new thread then pokes the original thread with the results when it is not busy), events (similar to bukkit events, or javafx/swing events), and simply doing the work and handling the results entirely on another thread are all ways to handle asynchronous programming.

    Concurrent programming is slightly different, instead of a single main thread that creates small, temporary threads as needed, it usually means a variable number of "main" threads working cooperatively to handle heavy workloads. They are not really usable with minecraft due to the heavy reliance on the single main server thread.

    And all this leads to thread safety.
    If two threads are running at the same time, they can both try to access a variable at the same time. This is fine if they just read from it (which you can ensure with the final keyword), but if they try changing it, or one changes it as the other reads it, you get "undefined behavior", which means weird shit happens. Anything from bizarre glitches to program crash.

    Best practice is to have each thread only operate on its own data, but there are ways to share data between threads if needed, the synchronized keyword and various collection classes that allow multiple threads to use them at the same time.

    As for how you make a thread, there is a class simply called Thread, which you can extend, it has the methods public void run(), and public void start(), which causes a new thread to be created and the run method (which contains whatever you want the thread to do) to be called on that thread.
    https://docs.oracle.com/javase/tutorial/essential/concurrency/runthread.html
     
    • Informative Informative x 3
  9. @FlyingLlama
    Isn't a Thread just the equivalent of a BukkitRunnable or BukkitTask in the BukkitAPI? Shouldn't I just use one of them instead?

    And if not, to use a Thread how could I return an Object? In order to override the start and run methods I cannot just change the return type to OfflinePlayer. Maybe I could cheat by creating an instance variable with a getter?
     
  10. Not exactly.
    Those are scheduled tasks which can be run on the main thread or on another thread or you use the runAsynchronously method to start them. They are simple to use for making a repeating task. A normal getter and setter makes no differenc to thread safety (imo they are terrible program clutter and utterly useless if you are not making an api - never use them in internal code without good reason), it still has to access the variable, just with another step.
    There are ways of writing synchronized getters and setters that safely access variables across threads, you can find tutorials for them.

    The easiest way for simple stuff is to simply have an atomic variable (meaning a type that is updated in one operation and can never be observed in a half-changed state: boolean, int, byte, short, char, float, but not long or double, or any Object), and use the value to control which thread has safe access or if the variable is being accessed currently.
    For a few heavy tasks, i use a boolean that switches to true when the thread is done, letting the main thread (which checks every few seconds) know it is time to safely collect the processed data. For anything more complex, callbacks or something like Aikar's taskchain API (pretty advanced and messy stuff) is a better choice.
     
    • Informative Informative x 1
  11. I don't really understand what you're saying but here's what I interpreted as how I should code the thread.
    Code (Java):
    public class OfflinePlayerThread extends Thread {

        private final String name;

        private boolean done = false;
        private UUID uuid;

        public OfflinePlayerThread(String name) {
            this.name = name;
        }

        public void run() {
            OfflinePlayer player = Bukkit.getOfflinePlayer(this.name);
            if (player.hasPlayedBefore()) {
                this.uuid = player.getUniqueId();
            } else {
                this.uuid = null;
            }
            this.done = true;
        }

        public UUID getUUID() {
            return this.uuid;
        }

        public boolean isDone() {
            return this.done;
        }

    }
    And modified code of the EssentialsHook command listener:
    Code (Java):
                        final UUID uuid;
                        Player p = Bukkit.getPlayer(arg2);

                        if (p != null) {
                            uuid = p.getUniqueId();
                        } else {
                            OfflinePlayerThread offlinePlayerThread = new OfflinePlayerThread(arg2);
                            offlinePlayerThread.start();

                            new BukkitRunnable() {
                                @Override
                                public void run() {
                                    if (offlinePlayerThread.isDone()) {
                                        cancel();
                                    }
                                }
                            }.runTaskTimer(this.plugin, 10L, 10L);
                            uuid = offlinePlayerThread.getUUID();
                        }
     
    #11 Roree, Apr 25, 2017
    Last edited: Apr 25, 2017
  12. PlayerCommandPreprocessEvent is called quite often, why don't you use CommandExecutor?
     
  13. @7rory768 Exactly, but threads actually have an isAlive() method natively, that checks if the thread has finished, so no need for the boolean.

    And that wont do what you think. The uuid field will never be set, because the runnable keeps a copy of it (it goes out of scope as soon as the event is exited, but the runnable still has a reference to it), and the main thread will be far past that point by the time the thread is finished and the runnable runs.

    If you need the uuid right away, freezing the server until it is returned is the only option, otherwise you have to wait and do something with it later, without any promise of exactly when 'later' is.
     
    • Winner Winner x 1
  14. Could you elaborate? If I register the ban, tempban, and ipban command in my plugin.yml wont it try to override the command registered by essentials, or is that just a misconception I have?

    How long can later be? The EssentialsHook's purpose is to unfreeze a player if they are banned while frozen, so it wouldn't really matter how long it took unless it was an extremely long time. And if I were to implement it would I just put the rest of the methods code inside the "if (thread is finished)" block?
     
  15. I'd just like to rather hint you towards the #async method in TaskChain (Aikar's thingy. Really worth using)
     
  16. Might be worth trying... if I knew how to use Maven in my IntelliJ projects
     
  17. Learn Gradle instead if you don't know either. Maven is easier, yeah, but not as powerful and is going downhill :p
     
  18. Later would probably be under a second, in this case. A very long time in a program, but not an important delay otherwise. Just have the thread start a new synchronous (on the main thread) task that unfreezes the player if needed, using a BukkitRunnable or the taskchain API, or some other similar beast. These handle the details of getting back to the main thread for you (basically the second thread says "hey main thread, do this", main thread eventually checks, notices it and does it - but it has to be actively looking for it).

    As for why you need to do it synchronously instead of on the async thread, it is a bad idea to do any interactions with the bukkit api or NMS from anything but the main thread, unless you are very careful and understand thread safety. All sorts of weird stuff can happen.
     
  19. So you're saying there's no way to asynchronously obtain the offline player meaning I cant fix the issue at all? You're confusing me because you told me to asynchronously get the offline player so it isn't frozen on the main thread but now you say I shouldn't do that because it uses the Bukkit API. Did you change your mind, or am I just misunderstanding you?
     
  20. sothatsit

    Patron

    Instead of starting another thread yourself, you can use
    Code (Text):
    Bukkit.getScheduler().runTaskAsynchronously(...);
    To run code asynchronously in which you get the offline player's uuid, then once it has completed use
    Code (Text):
    Bukkit.getScheduler().runTask(...);
    To run code back on the main thread to avoid concurrency issues.