Merge pull request Netflix#119 from IgorPerikov/issue#106_percentile_…

…window_speed_up make addSample method idempotent for percentile window
IgorPerikov · Jun 8, 2019 · f8c115b · f8c115b
2 parents ef49079 + 29f7836
commit f8c115b
Show file tree

Hide file tree

Showing 4 changed files with 66 additions and 43 deletions.
diff --git a/...ain/java/com/netflix/concurrency/limits/limit/window/ImmutablePercentileSampleWindow.java b/...ain/java/com/netflix/concurrency/limits/limit/window/ImmutablePercentileSampleWindow.java
@@ -15,51 +15,55 @@
  */
 package com.netflix.concurrency.limits.limit.window;
 
-import java.util.ArrayList;
-import java.util.Comparator;
-import java.util.List;
+import java.util.Arrays;
 import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLongArray;
 
 class ImmutablePercentileSampleWindow implements SampleWindow {
     private final long minRtt;
     private final int maxInFlight;
     private final boolean didDrop;
-    private final List<Long> observedRtts;
+    private final AtomicLongArray observedRtts;
+    private final int sampleCount;
     private final double percentile;
 
-    ImmutablePercentileSampleWindow(double percentile) {
+    ImmutablePercentileSampleWindow(double percentile, int windowSize) {
         this.minRtt = Long.MAX_VALUE;
         this.maxInFlight = 0;
         this.didDrop = false;
-        this.observedRtts = new ArrayList<>();
+        this.observedRtts = new AtomicLongArray(windowSize);
+        this.sampleCount = 0;
         this.percentile = percentile;
     }
 
-    ImmutablePercentileSampleWindow(
+    private ImmutablePercentileSampleWindow(
             long minRtt,
             int maxInFlight,
             boolean didDrop,
-            List<Long> observedRtts,
+            AtomicLongArray observedRtts,
+            int sampleCount,
             double percentile
     ) {
         this.minRtt = minRtt;
         this.maxInFlight = maxInFlight;
         this.didDrop = didDrop;
         this.observedRtts = observedRtts;
+        this.sampleCount = sampleCount;
         this.percentile = percentile;
     }
 
     @Override
     public ImmutablePercentileSampleWindow addSample(long rtt, int inflight, boolean didDrop) {
-        // TODO: very naive
-        // full copy in order to fulfill side-effect-free requirement of AtomicReference::updateAndGet
-        List<Long> newObservedRtts = new ArrayList<>(observedRtts);
-        newObservedRtts.add(rtt);
+        if (sampleCount >= observedRtts.length()) {
+            return this;
+        }
+        observedRtts.set(sampleCount, rtt);
         return new ImmutablePercentileSampleWindow(
                 Math.min(minRtt, rtt),
                 Math.max(inflight, this.maxInFlight),
                 this.didDrop || didDrop,
-                newObservedRtts,
+                observedRtts,
+                sampleCount + 1,
                 percentile
         );
     }
@@ -71,10 +75,18 @@ public long getCandidateRttNanos() {
 
     @Override
     public long getTrackedRttNanos() {
-        observedRtts.sort(Comparator.naturalOrder());
-        int rttIndex = (int) Math.round(observedRtts.size() * percentile);
+        if (sampleCount == 0) {
+            return 0;
+        }
+        long[] copyOfObservedRtts = new long[sampleCount];
+        for (int i = 0; i < sampleCount; i++) {
+            copyOfObservedRtts[i] = observedRtts.get(i);
+        }
+        Arrays.sort(copyOfObservedRtts);
+
+        int rttIndex = (int) Math.round(sampleCount * percentile);
         int zeroBasedRttIndex = rttIndex - 1;
-        return observedRtts.get(zeroBasedRttIndex);
+        return copyOfObservedRtts[zeroBasedRttIndex];
     }
 
     @Override
@@ -84,7 +96,7 @@ public int getMaxInFlight() {
 
     @Override
     public int getSampleCount() {
-        return observedRtts.size();
+        return sampleCount;
     }
 
     @Override
@@ -98,7 +110,7 @@ public String toString() {
                 + "minRtt=" + TimeUnit.NANOSECONDS.toMicros(minRtt) / 1000.0
                 + ", p" + percentile + " rtt=" + TimeUnit.NANOSECONDS.toMicros(getTrackedRttNanos()) / 1000.0
                 + ", maxInFlight=" + maxInFlight
-                + ", sampleCount=" + observedRtts.size()
+                + ", sampleCount=" + sampleCount
                 + ", didDrop=" + didDrop + "]";
     }
 }
diff --git a/.../main/java/com/netflix/concurrency/limits/limit/window/PercentileSampleWindowFactory.java b/.../main/java/com/netflix/concurrency/limits/limit/window/PercentileSampleWindowFactory.java
@@ -19,18 +19,20 @@
 
 public class PercentileSampleWindowFactory implements SampleWindowFactory {
     private final double percentile;
+    private final int windowSize;
 
-    private PercentileSampleWindowFactory(double percentile) {
+    private PercentileSampleWindowFactory(double percentile, int windowSize) {
         this.percentile = percentile;
+        this.windowSize = windowSize;
     }
 
-    public static PercentileSampleWindowFactory of(double percentile) {
+    public static PercentileSampleWindowFactory of(double percentile, int windowSize) {
         Preconditions.checkArgument(percentile > 0 && percentile < 1.0, "Percentile should belong to (0, 1.0)");
-        return new PercentileSampleWindowFactory(percentile);
+        return new PercentileSampleWindowFactory(percentile, windowSize);
     }
 
     @Override
     public ImmutablePercentileSampleWindow newInstance() {
-        return new ImmutablePercentileSampleWindow(percentile);
+        return new ImmutablePercentileSampleWindow(percentile, windowSize);
     }
 }
diff --git a/...st/java/com/netflix/concurrency/limits/limit/window/ImmutableAverageSampleWindowTest.java b/...st/java/com/netflix/concurrency/limits/limit/window/ImmutableAverageSampleWindowTest.java
@@ -25,16 +25,16 @@ public class ImmutableAverageSampleWindowTest {
 
     @Test
     public void calculateAverage() {
-        ImmutableAverageSampleWindow window = new ImmutableAverageSampleWindow();
+        SampleWindow window = new ImmutableAverageSampleWindow();
         window = window.addSample(bigRtt, 1, false);
         window = window.addSample(moderateRtt, 1, false);
         window = window.addSample(lowRtt, 1, false);
         Assert.assertEquals((bigRtt + moderateRtt + lowRtt) / 3, window.getTrackedRttNanos());
     }
 
     @Test
-    public void droppedSampleShouldNotChangeTrackedAverage() {
-        ImmutableAverageSampleWindow window = new ImmutableAverageSampleWindow();
+    public void droppedSampleShouldChangeTrackedAverage() {
+        SampleWindow window = new ImmutableAverageSampleWindow();
         window = window.addSample(bigRtt, 1, false);
         window = window.addSample(moderateRtt, 1, false);
         window = window.addSample(lowRtt, 1, false);

diff --git a/...java/com/netflix/concurrency/limits/limit/window/ImmutablePercentileSampleWindowTest.java b/...java/com/netflix/concurrency/limits/limit/window/ImmutablePercentileSampleWindowTest.java
@@ -18,37 +18,46 @@
 import org.junit.Assert;
 import org.junit.Test;
 
+
 public class ImmutablePercentileSampleWindowTest {
-    private final long slowestRtt = 5000;
+    private final long bigRtt = 5000;
     private final long moderateRtt = 500;
-    private final long fastestRtt = 10;
+    private final long lowRtt = 10;
 
     @Test
     public void calculateP50() {
-        ImmutablePercentileSampleWindow window = new ImmutablePercentileSampleWindow(0.5);
-        window = window.addSample(slowestRtt, 1, false);
-        window = window.addSample(moderateRtt, 1, false);
-        window = window.addSample(fastestRtt, 1, false);
+        SampleWindow window = new ImmutablePercentileSampleWindow(0.5, 10);
+        window = window.addSample(bigRtt, 0, false);
+        window = window.addSample(moderateRtt, 0, false);
+        window = window.addSample(lowRtt, 0, false);
         Assert.assertEquals(moderateRtt, window.getTrackedRttNanos());
     }
 
     @Test
-    public void droppedSampleShouldNotChangeTrackedRtt() {
-        ImmutablePercentileSampleWindow window = new ImmutablePercentileSampleWindow(0.5);
-        window = window.addSample(slowestRtt, 1, false);
-        window = window.addSample(moderateRtt, 1, false);
-        window = window.addSample(fastestRtt, 1, false);
-        window = window.addSample(slowestRtt, 1, true);
-        Assert.assertEquals(moderateRtt, window.getTrackedRttNanos());
+    public void droppedSampleShouldChangeTrackedRtt() {
+        ImmutablePercentileSampleWindow window = new ImmutablePercentileSampleWindow(0.5, 10);
+        window = window.addSample(lowRtt, 1, false);
+        window = window.addSample(bigRtt, 1, true);
+        window = window.addSample(bigRtt, 1, true);
+        Assert.assertEquals(bigRtt, window.getTrackedRttNanos());
     }
 
     @Test
     public void p999ReturnsSlowestObservedRtt() {
-        ImmutablePercentileSampleWindow window = new ImmutablePercentileSampleWindow(0.999);
-        window = window.addSample(slowestRtt, 1, false);
+        SampleWindow window = new ImmutablePercentileSampleWindow(0.999, 10);
+        window = window.addSample(bigRtt, 1, false);
+        window = window.addSample(moderateRtt, 1, false);
+        window = window.addSample(lowRtt, 1, false);
+        Assert.assertEquals(bigRtt, window.getTrackedRttNanos());
+    }
+
+    @Test
+    public void rttObservationOrderDoesntAffectResultValue() {
+        SampleWindow window = new ImmutablePercentileSampleWindow(0.999, 10);
         window = window.addSample(moderateRtt, 1, false);
-        window = window.addSample(fastestRtt, 1, false);
-        window = window.addSample(slowestRtt, 1, true);
-        Assert.assertEquals(slowestRtt, window.getTrackedRttNanos());
+        window = window.addSample(lowRtt, 1, false);
+        window = window.addSample(bigRtt, 1, false);
+        window = window.addSample(lowRtt, 1, false);
+        Assert.assertEquals(bigRtt, window.getTrackedRttNanos());
     }
 }