improve average performance of long task timer for out-of-order start…

…/stop of many tasks

micrometer-core/src/main/java/io/micrometer/core/instrument/internal/DefaultLongTaskTimer.java

@@ -27,8 +27,7 @@
 
 import java.util.*;
 import java.util.concurrent.ArrayBlockingQueue;
-import java.util.concurrent.ConcurrentLinkedDeque;
-import java.util.concurrent.CopyOnWriteArrayList;
+import java.util.concurrent.ConcurrentSkipListSet;
 import java.util.concurrent.TimeUnit;
 import java.util.function.Consumer;
 import java.util.stream.Collectors;
@@ -37,18 +36,23 @@
 public class DefaultLongTaskTimer extends AbstractMeter implements LongTaskTimer {
 
     /**
-     * Preferring {@link ConcurrentLinkedDeque} over {@link CopyOnWriteArrayList} here
+     * Preferring {@link ConcurrentSkipListSet} over other concurrent collections
      * because...
      * <p>
      * Retrieval of percentile values will be O(N) but starting/stopping tasks will be
-     * O(1). Starting/stopping tasks happen in the same thread as the main application
-     * code, where publishing generally happens in a separate thread. Also, shipping
-     * client-side percentiles should be relatively uncommon.
+     * O(log(N)). Starting/stopping tasks happen in the same thread as the main
+     * application code, where publishing generally happens in a separate thread. Also,
+     * shipping client-side percentiles should be relatively uncommon.
+     * <p>
+     * Tasks are naturally ordered at time of insertion, but task completion/removal can
+     * happen out-of-order, that causes removal of tasks from the middle of the list. A
+     * queue would provide O(1) insertion, but O(N) removal in average. A skip-list
+     * provides O(log(N)) average across all tasks.
      * <p>
      * Histogram creation is O(N) for both the queue and list options, because we have to
      * consider which bucket each active task belongs.
      */
-    private final Deque<SampleImpl> activeTasks = new ConcurrentLinkedDeque<>();
+    private final NavigableSet<SampleImpl> activeTasks = new ConcurrentSkipListSet<>();
 
     private final Clock clock;
 
@@ -92,7 +96,9 @@ public DefaultLongTaskTimer(Id id, Clock clock, TimeUnit baseTimeUnit,
     @Override
     public Sample start() {
         SampleImpl sample = new SampleImpl();
-        activeTasks.add(sample);
+        if (!activeTasks.add(sample)) {
+            throw new IllegalStateException();
+        }
         return sample;
     }
 
@@ -108,8 +114,13 @@ public double duration(TimeUnit unit) {
 
     @Override
     public double max(TimeUnit unit) {
-        Sample oldest = activeTasks.peek();
-        return oldest == null ? 0.0 : oldest.duration(unit);
+        try {
+            Sample oldest = activeTasks.first();
+            return oldest.duration(unit);
+        }
+        catch (NoSuchElementException e) {
+            return 0.0;
+        }
     }
 
     @Override
@@ -215,7 +226,7 @@ public HistogramSnapshot takeSnapshot() {
                 (ps, scaling) -> ps.print("Summary output for LongTaskTimer histograms is not supported."));
     }
 
-    class SampleImpl extends Sample {
+    class SampleImpl extends Sample implements Comparable<SampleImpl> {
 
         private final long startTime;
 
@@ -249,6 +260,15 @@ public String toString() {
                     + ", " + "duration(nanos)=" + durationInNanoseconds + ", " + "startTimeNanos=" + startTime + '}';
         }
 
+        @Override
+        public int compareTo(DefaultLongTaskTimer.SampleImpl o) {
+            int startCompare = Long.compare(startTime, o.startTime);
+            if (startCompare == 0) {
+                return Long.compare(hashCode(), o.hashCode());
+            }
+            return startCompare;
+        }
+
     }
 
 }