Util ExonRunnable - A new Runnable!

Decided to not share, good luck.​

Reserved for feature updates.

EDIT by Moderator: merged posts, please use the edit button instead of double posting.

 

Did you benchmark it?

 

Memory Consumption dictates, most times,the execution time as well. Those being said, the timers, in order of mem consumption are: Java Timer < ExonTimer < BukkitRunnable. Tests have been made with 10000 timers of each kind, for the ExonTT and ExonRT.

Still dont believe me?
Step I: Do what I wrote first, check the memory usage.
Step II: Decompile bukkit / ExonTimer
Step III: Check BukkitRunnable's implementation, check mine.

 

No, it does not.

You have numerous mistakes pertaining to performance in your code. Not only that, I don't understand how wrapping java.util.Timer (which, by the way, is a legacy class, there's no reason you should be using it over a handrolled implementation or ScheduledExecutorService) allows you to think that your code is more "performant" than the Bukkit scheduler.

The most damning aspect about your usage of Timer is that it requires tasks executed to be thread-safe. The limited Bukkit methods that are allowed to be executed by the Timer's background thread are incredibly limited, making this library essentially useless because it doesn't allow a sync option.

Your statement only works because the timers that you created are thrown away as soon as you create them. You have also created a Timer for every single task.

Here is a code snippet from ExonTaskTimer.class:

Code:

public void startTimer()
  {
    Timer T = new Timer();
    ...
}

Oh. Let's read more on what a Timer does:

Oh wow! Let's just confirm with the source just to make sure that it does exactly as said.

BIG performance penalty. You are using an outdated class, making use of unbounded thread creation. I feel that the link I have given you here will me more helpful than me reiterating why using a Timer is bad, I strongly recommend a good read into that.

Code:

public void stop()
  {
    if (this.Timer != null) {
      this.Timer.cancel();
    }
  }

This is your concept of stopping a task. As you can see, you have a completely useless nullcheck. Your timer is never allowed to be null if the task was scheduled correctly. If your task is not scheduled correctly, either it is because you have a redundant if check in your constructor, which I will mention below, or your unbounded thread creation has reached a ceiling and the task fails. Either way, the application needs to exit or you need to fix your code, meaning you don't need your null check.

Even if your timer could potentially be null, this is all the more reason to use constructor initialized final fields in order to prevent that, or alternatively, don't even expose the stop method because it should be handled by the ExonTimerStrorer anyways. In fact, why have you even exposed this method if the condition already does the exact same thing?

Code:

if (this.OnlineTimers.containsKey(Integer.valueOf(timerId)))
    {
      CustomTimer Timer = (CustomTimer)this.OnlineTimers.get(Integer.valueOf(timerId));
      Timer.stop();
  
      this.OnlineTimers.remove(Integer.valueOf(timerId));
    }

Just... What is this? You could quite literally perform this with a single HashMap operation:

Code:

CustomTimer t = map.remove(timerId);
if (t != null) t.stop();

That is TWO LINES. It is more efficient, more understandable, and doesn't use redundant operations. I will not dispute your reasons for using Integer.valueOf, since that is most likely the decompiler explicitly displaying autobox, but if you do have that, just let autobox do its job. Performance penalty for the extra operations.

Code:

public ArrayList<Integer> getAllTimers()
  {
    ArrayList<Integer> timerIds = new ArrayList();
    for (Integer k : this.OnlineTimers.keySet()) {
      timerIds.add(k);
    }
    return timerIds;
  }

I quite literally don't have a clue what you are trying to achieve here... I don't get why you can't do:

Code:

return new ArrayList(OnlineTimers.keySet());

I mean it's one line compared to 5... It's also completely redundant. Whilst not a performance penalty, you could easily just get rid of copying alltogether, I mean, as tasks get cancelled index based searching is useless because the index of each integer becomes arbitrary and you would want a faster remove/contains time, right? Should you choose to copy, it should be to a HashSet instead.

Code:

if ((taskToRun != null) && (tickTime > 0L))

Well, why are you checking if taskToRun isn't null? I don't understand why you would want to check that, performing a useless operation that makes no semantic sense, because the second condition will never be false unless someone actually does use a 0L interval. Performance penalty.

Finally, you have a memory leak when cancelling tasks...

Code:

if (ExonConditionTaskTimer.this.conditionCheck()) {
          cancel();

You forgot to remove the task from the ExonTimerStorer when cancelling condition tasks.

You have also made several design decisions that I find completely baffling...

I don't understand, if you don't want API users to type those names, why don't you:

a. Get rid of the long named classes
b. Find a shorter name
c. Or use a short interface and a static factory

Um what?

Some tasks needs to stay in sync with the server tick in order to function properly. Granted, you could use 50ms instead, but there will be a small scheduling offset which causes the task to run at a different period than the server tick.

Additionally, the server's time unit is in ticks, it is designed so that long running tasks or server-intensive processes don't eat up the CPU's time slices and allow a more homogeneous run cycle. Giving the opportunity to have smaller than 50 ms cycle times allows developers who don't know how the Minecraft server works interferes with resource sharing and break execution policies that are set by the server.

Here is a snippet of code from one of your classes:

Code:

public class ExonTaskTimer
  implements CustomTimer
{
  protected int timerid;
  protected long tickTime;
  protected Timer Timer = null;
  protected ExonRunTask taskToRun;

WRONG WRONG WRONG WRONG WRONG WRONG WRONG

This class needs to be final. If not, the fields need to be private, and you should know why: extensibility causes problems. Sooner or later, you have a knucklehead going around doing this:

Code:

class ExtendedExonTaskTimer extends ExonTaskTimer {
    {
        timerid = 69;
    }
}

Here is another place where you have used a publically visible field that should not be public (ExonTimerStorer.class):

Code:

public static final ExonTimerStorer ExonTimers = new ExonTimerStorer();

You have also failed to account for, again - thread safety. taskToRun needs to be final or else the compiler can't guarantee that you have a well-formed execution. Additionally, you still have the danger of plugins extending your class to again screw with the superclass fields.

As far as the singleton design for your class goes, I am not in a place to comment, however, I don't see why your collection of tasks is static, it is unnecessary for singletons...

Undescriptive, unclear, incredibly vague, states the obvious. What does the method do? What is the purpose of the return type? Why must you have this? Yes, it returns a boolean, which is 99% of the time, a "condition". Using "condition" makes it easy to get mistakes and not know why because it is unclear what this method does with the return value, or even what circumstances constitute the necessity to return true or false.

Instead, why isn't it named "isCancelled()"? "canTerminate"? "canContinue"? I would never of guessed that returning true will cancel the timer. Do rename the method appropriately.

Finally, you have made this a very inflexible and lacking of features that is offered by the vastly superior BukkitRunnable. You have failed to provide scheduled execution which does not repeat, and also failed to provide immediate execution, both which are provided by BukkitRunnable. You have failed to follow correct and defensive programming procedures of a properly designed API and failed to consider task thread-safety. Your code is not open source, your code is not documented, and requires examples in order to understand how it works. This, all is handled by the BukkitRunnable.

PS No offense, but please do some research or at least present some quantifiable data about how your sacrifices in design and thread safety justify a "theoretical" performance gain in ExonRunnable

Edit 1: I see how this could apply to Bukkit but I also feel like not incorporating the use of the Bukkit API in the source makes this less applicable to Bukkit. Again, there's nothing wrong with that, but making it more Bukkit relevant could make it more useful (?)

Edit 2: Ok, I will benchmark this myself.

Synopsis: Test the code of both the ExonRunnable and BukkitRunnable code for "Repeating Task" and obtain nanosecond precision numbers that can be used to compare the execution latency of using either and attempt to recommend a performant solution to the dilemma between the two. The ExonRunnable will use the "full name" in order to avoid inheritance overhead.

Methodology: Running a benchmark in a complex environment such as within the context of a Spigot server will make it difficult to obtain accurate results. Using various techniques described, I will attempt to make this test as fair and as accurate as possible.

Because the term "performance" is incredibly general and does not provide a description to the aspect in which performance matters, the assumption is made that performance means the code that is expected to be executed will be executed as quickly as possible - that is the code that is scheduled inside of the corresponding runnable. This is still an overly broad definition of the term "performance" so the test will include three types of tasks that are submitted: heterogeneous, where the workload varies; fast homogeneous, where the tasks are similarly short-running; and long homogeneous, where the tasks are similarly long-running.

The trials used in this test will be 100_000 (one hundred thousand) cycles for the warmup period, and 3_000_000 (three million) cycles for the measurement period. The test will measure the average time taken to execute each given task. No profiler will be used as to prevent offsets of time in different safepoints in the code. The final data will be displayed in a three column, two row table which represents each type of task executed and which runnable was used to execute the task. A "generalization" can be made for a 2/3 performance gain, and a stronger generalization can be made by a 3/3 performance gain.

The work done will use a special modified class called "StatefulOp" which is an Apache 2.0 Licensed file taken from AgentTroll/Fastest-Fing-Data-Structures and has a random number generator useful for creating work for the heterogeneous tasks, which will be passed to the burn method used to simulate a real task. Each task will be given a 50 ms delay and 50 ms interval. The time will be taken after the task runs 500 times. Since each task is given 500 cycles, the amount of iterations run by the test would be cycles / 500. The time will be taken after 500 runs have passed. In order to prevent state consistency overhead, we will create a new StatefulOp each run of this method.

Each test will take place on a separate instance of git-Spigot-f928e7a-994b2aa (MC: 1.8.7) (Implementing API version 1.8.7-R0.1-SNAPSHOT). The instances will never run simultaneously on the machine, which is a MacBook Pro with the following specs:

Code:

Running Mac OS X version 10.10.3 arch x86_64
Java version 1.8.0_45 JVM Oracle Corporation
Java flags: -Didea.launcher.port=7533 -Didea.launcher.bin.path=/Applications/IntelliJ IDEA 14.app/Contents/bin -Dfile.encoding=UTF-8
Memory total 17179869184 bytes, usable 9059049472 bytes
VM memory free 223826768 bytes, max 3817865216 bytes, total 257425408 bytes
CPUs (8):
  Intel(R) Core(TM) i7-4770HQ CPU @ 2.20GHz
  Intel(R) Core(TM) i7-4770HQ CPU @ 2.20GHz
  Intel(R) Core(TM) i7-4770HQ CPU @ 2.20GHz
  Intel(R) Core(TM) i7-4770HQ CPU @ 2.20GHz
  Intel(R) Core(TM) i7-4770HQ CPU @ 2.20GHz
  Intel(R) Core(TM) i7-4770HQ CPU @ 2.20GHz
  Intel(R) Core(TM) i7-4770HQ CPU @ 2.20GHz
  Intel(R) Core(TM) i7-4770HQ CPU @ 2.20GHz
Disks (1):
  Macintosh HD (/) cap 249795969024 bytes, usable 205422809088 bytes

Code:
TBC

tbc code (open) tbc code (close)

Code:

package pckge;

import CustomTimer.TimerTypes.FullNames.RepeatedTimers.ExonConditionRepeatedTimer;
import CustomTimer.Util.Interfaces.ExonRunCondition;
import CustomTimer.Util.Interfaces.ExonRunTask;
import com.google.common.collect.Lists;
import org.bukkit.plugin.java.JavaPlugin;

import java.util.List;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.atomic.AtomicLong;
import java.util.logging.Logger;

public class Benchmark extends JavaPlugin {
    private static final int CYCLE = 10;
    private static final int WARM_UP = 500 / CYCLE;
    private static final int MEASURE =  3000/ CYCLE;
    private static final String TEST_NAME = "Exon_Heterogeneous";

    @Override public void onEnable() {
        try {
            Thread.sleep(5000);
        } catch (InterruptedException e) {
            e.printStackTrace();
        }

        Logger c = getLogger();
        c.info("Starting test: " + TEST_NAME);

        double result = runTest();
        c.info("Results for " + TEST_NAME + ": " + result + " ns/op*500");
    }

    public void runTask(AtomicLong result) {
        StatefulOp burner = new StatefulOp();
        CountDownLatch latch = new CountDownLatch(1);
        new ExonConditionRepeatedTimer(new ExonRunTask() {
            @Override
            public void run() {
                burner.burn(burner.prng() & 120);
            }
        }, new ExonRunCondition() {
            private int runs;

            @Override public boolean condition() {
                boolean b = ++runs == 500;
                if (b) {
                    result.set(System.nanoTime());
                    latch.countDown();
                }

                return b;
            }
        }, 50L, 50L);

        try {
            latch.await();
        } catch (InterruptedException e) {
            e.printStackTrace();
        }
    }

    public double runTest() {
        AtomicLong warmUp = new AtomicLong();
        for (int i = 0; i < WARM_UP; i++) {
            runTask(warmUp); // allow the value to be thrown away, we don't need that
        }

        List<Long> times = Lists.newArrayList();

        AtomicLong measurement = new AtomicLong();
        for (int i = 0; i < MEASURE; i++) {
            long start = System.nanoTime();
            runTask(measurement);
            times.add(measurement.get() - start);

            measurement.set(0);
        }

        if (times.size() != MEASURE) {
            throw new IllegalStateException("The samples taken do not match the expected amount");
        }

        long total = 0L;
        for (long time : times) {
            total += time;
        }

        return total / ((double) MEASURE);
    }

    /*
     * Copyright 2015 Pierre C
     * FFDSJ - Fast Fing Data Structures Java
     *
     * Licensed under the Apache License, Version 2.0 (the "License");
     * you may not use this file except in compliance with the License.
     * You may obtain a copy of the License at
     *
     *     http://www.apache.org/licenses/LICENSE-2.0
     *
     * Unless required by applicable law or agreed to in writing, software
     * distributed under the License is distributed on an "AS IS" BASIS,
     * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
     * See the License for the specific language governing permissions and
     * limitations under the License.
     */
    // Benchmark - Remove class javadoc
    public static class StatefulOp {
        // Benchmark - remove "random" field and javadoc
        /** The value for burning off CPU */
        private int x = prng();

        // Only instantiable in this class
        StatefulOp() {
        }

        public int prng() { // Benchmark - private -> public, remove parameter
            // Benchmark - move int cast, parameter -> x, new Object().hashCode() -> burn(37)
            return (int) (Math.abs((System.currentTimeMillis() - (0x61732 & x))) / burn(37));
        }

        // Benchmark - remove method javadoc, return result
        public int burn(int i) {
            for (; i > 0; i--) {
                x |= (x ^ 0xF457) & (i ^ 0x38D3);
            }

            return x;
        }
    }
}

 

@xTrollxDudex
Someone had a lot of spare time

 

@xTrollxDudex just: wow