Solved BukkitRunnable, new for new timer or one timer for all

Discussion in 'Spigot Plugin Development' started by Nuubles, Mar 7, 2019.

?

Which choise is the Best for efficiency (both memory and cpu)

  1. Create multiple threads every n:th tick to run different tasks (current solution)

    80.0%
  2. Create one thread every n:th tick which runs different tasks one by one

    0 vote(s)
    0.0%
  3. Use one timer which runs all threads one by one (risk of cluttering and tps drops)

    20.0%
  1. I have a Core plugin under development, which is going to be taking care of dozens of simple tasks (animated timed bossbar, automatic timers, translation engine, entityhandlers, semiautomatic player cache, itemmanager, animated 3D particle object drawer, graph data structures, location math, semiautomatic inventory menu which can be used to prevent inventory closing etc., semiautomatic string -> time conversion, "custom" sounds etc. etc.

    The problem I currently am facing is the following: when registering timers, should they all be run under the same timer or multiple different timers which are created each time the timer is run? There may be up to N timers given to this class so the amount of timers may be in the tens or hundreds. The code used is given below:

    Code (Java):
    package net.karanteeni.core.timers;

    import java.util.HashMap;
    import java.util.Map;
    import java.util.Map.Entry;
    import java.util.logging.Level;

    import org.bukkit.Bukkit;
    import org.bukkit.plugin.Plugin;
    import org.bukkit.scheduler.BukkitRunnable;

    import net.karanteeni.core.KaranteeniCore;

    public class KaranteeniTimerInitiater {
        private final Map<KaranteeniTimer, Integer> listeners = new HashMap<KaranteeniTimer, Integer>();
        private final BukkitRunnable timer;
        private int tickCount = 0;
        private Plugin plugin = KaranteeniCore.getPlugin(KaranteeniCore.class);
     
        /**
         * Creates and starts the timer
         */

        public KaranteeniTimerInitiater()
        {
            timer = new BukkitRunnable() {
                @Override
                public void run() {
                    runTimer();
                }
            };
         
            timer.runTaskTimerAsynchronously(plugin, 1, 1);
        }
     
        /**
         * Register a timer to be runned
         * @param timer timer which is called
         * @param tickLimit tickes between each call
         */

        public void registerTimer(KaranteeniTimer timer, int tickLimit) throws IllegalArgumentException
        {
            if(tickLimit > 0)
            {
                listeners.put(timer, tickLimit);
            }
            else
                throw new IllegalArgumentException("Illegal time! Has to be 1-N. " + tickLimit + " given.");
        }
     
        /**
         * Otetaan ajastin pois rekisteräinnistä
         * @param timer
         */

        public void unregisterTimer(KaranteeniTimer timer)
        {
            timer.timerStopped();
            listeners.remove(timer);
        }
     
        /**
         * Runs all timers once
         */

        public void runTimer()
        {
            for (Entry<KaranteeniTimer, Integer> entry : listeners.entrySet())
            {
                //Has there been X ticks since last call
                if(tickCount % entry.getValue() == 0)          
                {
                    //The main function of the timer
                    Bukkit.getScheduler().scheduleSyncDelayedTask(plugin,
                        new Runnable()
                        {
                            @Override
                            public void run()
                            {
                                try
                                {
                                    entry.getKey().runTimer();
                                }
                                catch(Exception e)
                                {
                                    plugin.getLogger().log(Level.WARNING, "An Error happened in timer runnable", e);
                                }
                            }
                        });
                }
                else
                {
                    //Runs the timerWait() function
                    Bukkit.getScheduler().scheduleSyncDelayedTask(plugin,
                        new Runnable()
                        {
                            @Override
                            public void run()
                            {
                                try
                                {
                                    entry.getKey().timerWait();
                                }
                                catch(Exception e)
                                {
                                    plugin.getLogger().log(Level.WARNING, "An Error happened in timer waiter runnable", e);
                                }
                            }
                        });
                }
            }
         
            if(tickCount == Integer.MAX_VALUE)
                tickCount = 0;
            ++tickCount;
        }
    }
     

    As you can see, a KaranteeniTimer object is given through the core plugin to this class which in turn takes care of running the given timer based on server tick speed (async to sync) (yes it's not ideal but I'll program a timer later which runs timers based on milliseconds and re-runs them of they have experienced delay up to the point of one runtime or something similiar)

    Would it be ideal to keep the code as it is now or should I make one timer which is run continuously and runs the given functions?

    If more information is needed about this code please tell me so I can tell.
     
    #1 Nuubles, Mar 7, 2019
    Last edited: Mar 7, 2019
  2. added a poll so it's possible to cast "short answers"
     
  3. I think you should use the same runnable to tick a task group like graphic effects or mob ticking
     
    • Useful Useful x 1
  4. Yea that's what I'm currently doing with my plugins (timer which creates multiple threads async to sync which may run one or more tasks/threads) :D Thanks for the tip anyway!

    Edit: nvm I misread first. I'll take a look into that approach; I'll probably add TaskGroup like enum into the timer which is used to split timer groups and do a bit of testing with performance etc. Currently the main timer though just creates new threads and does not run them in its own thread
     
    #4 Nuubles, Mar 13, 2019
    Last edited: Mar 13, 2019
  5. I’m really confused as to the purpose of this. Why not just use a regular sync timer? The scheduler already does error handling, the scheduler already ticks synchronously so it doesn’t even matter that your main loop is async (I’d even argue that it’s in fact detrimental). You also have no need for the max integer check because you can run your server continuously for 3 years (which you won’t) before it gets anywhere near the max value. Also, why are you storing a BukkitRunnable in a field? Shouldn’t you be keeping the BukkitTask instead?

    Pertaining to your original question, the performance impact of this will be negligible for small number of tasks, but you should put your task iteration inside the scheduler (as in schedule a single task to go through all the listeners rather than going through all the listeners to schedule a task).

    Finally, I will again reiterate that for sync tasks, the performance difference is negligible. You cannot get the same effect of polluting the system thread scheduler with sync tasks as you can with async tasks. The fact that you are trying to optimize this shows that either you have a poor understanding of the scheduler performance nuances or your performance data is vastly skewed or incorrect in some way.
     
    #5 xTrollxDudex, Mar 13, 2019
    Last edited: Mar 13, 2019
    • Useful Useful x 1
  6. Thanks for the information, I'll be testing all the methods/changes suggested in this thread during the weekend and will post some results here for future use. As you pointed out I do not have all the necessary knowledge regarding application threads and their performance nuances as I haven't had that kind of course (or received information) in uni yet. I had no idea for example that async tasks may pollute system thread schedulers and I'll be looking into that before modifying my code and testing performance.
     
  7. After some testing, here are my results:

    XXX-XX(exceptions)
    First number = amount of timers generated
    Seconds number = amount of one timer being run before removing the timer
    Exceptions = Timer was designed to generate exceptions

    Like you pointed out @xTrollxDudex , your solution seems faster, though like you mentioned, the differences are very small.

    I will be pasting below the code used (notice: tickCount will be removed in final version and runnable will be tested and most likely moved to own class to prevent null check and copying which was made to remove nullpointerexceptions. Will also check if BukkitTask timer is really needed or if reference is kept in bukkit)

    Note that the time may be skewed a bit as the code is run once every tick in these tests so the optimal time for 100 runs is 100 ticks -> 5000ms (not really sure how some timers got under that though, will probably produce a test later using wait() in timer or instead of running the timer N times I'll run the random number generator N times)

    [​IMG]

    Code (Java):
    class TimerTester implements KaranteeniTimer
        {
            private double val;
            private int runTimes = 0;
            private boolean printStart = false;
            private boolean printStop = false;
            private long startTime;
            private boolean runOnce = false;
       
            public TimerTester (boolean printStart, boolean printStop, int runTimes)
            {
                this.printStart = printStart;
                this.printStop = printStop;
                this.runTimes = runTimes;
            }
       
            @Override
            public void runTimer()
            {
                if(printStart)
                {
                    Bukkit.broadcastMessage("Timer started at:" + System.currentTimeMillis());
                    this.printStart = false;
                }
           
                if(!runOnce)
                {
                    this.startTime = System.currentTimeMillis();
                    this.runOnce = true;
                }
           
                Random rnd = new Random();
                val = rnd.nextDouble(); //Generate a random double value
                --runTimes;
           
                //Unregister timer to prevent infinite loop
                if(runTimes == 0)
                    KaranteeniPlugin.getTimerHandler().unregisterTimer(this);
            }

            @Override
            public void timerStopped()
            {
                //Print time when the timer was stopped
                if(printStop)
                {
                    Bukkit.broadcastMessage("Result: " + (System.currentTimeMillis() - this.startTime));
                }
            }

            @Override
            public void timerWait() { }
        }

    Code (Java):
    class TimerTesterException implements KaranteeniTimer
        {
            private double val;
            private int runTimes = 0;
            private boolean printStart = false;
            private boolean printStop = false;
            private long startTime;
            private boolean runOnce = false;
       
            public TimerTesterException (boolean printStart, boolean printStop, int runTimes)
            {
                this.printStart = printStart;
                this.printStop = printStop;
                this.runTimes = runTimes;
            }
       
            @Override
            public void runTimer()
            {
                if(printStart)
                {
                    Bukkit.broadcastMessage("Timer started at:" + System.currentTimeMillis());
                    this.printStart = false;
                }
           
                if(!runOnce)
                {
                    this.startTime = System.currentTimeMillis();
                    this.runOnce = true;
                }
           
                Random rnd = new Random();
                val = rnd.nextDouble(); //Generate a random double value
                --runTimes;
           
                //Unregister timer to prevent infinite loop
                if(runTimes == 0)
                    KaranteeniPlugin.getTimerHandler().unregisterTimer(this);
           
                int i = 100/0;
            }

            @Override
            public void timerStopped()
            {
                //Print time when the timer was stopped
                if(printStop)
                {
                    Bukkit.broadcastMessage("Result: " + (System.currentTimeMillis() - this.startTime));
                }
            }

            @Override
            public void timerWait() { }
        }

    Code (Java):
    private void testTimer(int timerCount, int runTimes, boolean exceptionTimer)
        {
            if(!exceptionTimer)
            for(int i = 0; i < timerCount; ++i)
            {
                if(i == 0)
                    KaranteeniPlugin.getTimerHandler().registerTimer(new TimerTester(true, true, runTimes), 1);
                else if(i+1 == timerCount)
                    KaranteeniPlugin.getTimerHandler().registerTimer(new TimerTester(true, true, runTimes), 1);
                else
                    KaranteeniPlugin.getTimerHandler().registerTimer(new TimerTester(false, false, runTimes), 1);
            }
            else
            for(int i = 0; i < timerCount; ++i)
            {
                if(i == 0)
                    KaranteeniPlugin.getTimerHandler().registerTimer(new TimerTesterException(true, true, runTimes), 1);
                else if(i+1 == timerCount)
                    KaranteeniPlugin.getTimerHandler().registerTimer(new TimerTesterException(true, true, runTimes), 1);
                else
                    KaranteeniPlugin.getTimerHandler().registerTimer(new TimerTesterException(false, false, runTimes), 1);
            }
        }

    Timer 1: https://pastebin.com/UkGSYMDT
    Timer 2: https://pastebin.com/zVvE0rua
    Timer 3: https://pastebin.com/uxR2Lfp5
    Timer 4: https://pastebin.com/dawgGe8g
    Timer 5: https://pastebin.com/xZssGPmu
     
    #7 Nuubles, Mar 15, 2019
    Last edited: Mar 15, 2019