From aba71f64aa601f936bdc104b79e3302210417bd9 Mon Sep 17 00:00:00 2001
From: fogninid <d.github.75db@f12o.eu>
Date: Mon, 14 Oct 2024 12:06:48 +0000
Subject: [PATCH] improve average performance of long task timer for
 out-of-order start/stop of many tasks

---
 .../internal/DefaultLongTaskTimer.java        | 34 +++++++++++++------
 1 file changed, 24 insertions(+), 10 deletions(-)

diff --git a/micrometer-core/src/main/java/io/micrometer/core/instrument/internal/DefaultLongTaskTimer.java b/micrometer-core/src/main/java/io/micrometer/core/instrument/internal/DefaultLongTaskTimer.java
index 7f56455158..6ccdffced4 100644
--- a/micrometer-core/src/main/java/io/micrometer/core/instrument/internal/DefaultLongTaskTimer.java
+++ b/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;
 
@@ -108,8 +112,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 +224,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 +258,11 @@ public String toString() {
                     + ", " + "duration(nanos)=" + durationInNanoseconds + ", " + "startTimeNanos=" + startTime + '}';
         }
 
+        @Override
+        public int compareTo(DefaultLongTaskTimer.SampleImpl o) {
+            return Long.compare(startTime, o.startTime);
+        }
+
     }
 
 }