General clean up, performance fixes

voldemort · Mar 1, 2012 · 311788b · 311788b
1 parent 5513200
commit 311788b
Show file tree

Hide file tree

Showing 3 changed files with 24 additions and 19 deletions.
diff --git a/src/java/voldemort/store/stats/Histogram.java b/src/java/voldemort/store/stats/Histogram.java
@@ -1,6 +1,7 @@
 package voldemort.store.stats;
 
-import voldemort.annotations.concurrency.NotThreadsafe;
+import voldemort.VoldemortException;
+import voldemort.annotations.concurrency.Threadsafe;
 
 import java.util.Arrays;
 
@@ -11,7 +12,7 @@
  *
  *
  */
-@NotThreadsafe
+@Threadsafe
 public class Histogram {
 
     private final int nBuckets;
@@ -45,7 +46,7 @@ protected void init() {
     /**
      * Reset the histogram back to empty (set all values to 0)
      */
-    public void reset() {
+    public synchronized void reset() {
         Arrays.fill(buckets, 0);
         size = 0;
     }
@@ -56,9 +57,11 @@ public void reset() {
      *
      * @param data The value to insert into the histogram
      */
-    public void insert(int data) {
+    public synchronized void insert(int data) {
         int index = findBucket(data);
-        assert(index != -1);
+        if(index == -1) {
+            throw new VoldemortException(data + " can't be bucketed, is invalid!");
+        }
         buckets[index]++;
         size++;
     }
@@ -70,7 +73,7 @@ public void insert(int data) {
      * @param quantile The percentile to find
      * @return Lower bound associated with the percentile
      */
-    public int getQuantile(double quantile) {
+    public synchronized int getQuantile(double quantile) {
         int total = 0;
         for(int i = 0; i < nBuckets; i++) {
             total += buckets[i];

diff --git a/src/java/voldemort/store/stats/RequestCounter.java b/src/java/voldemort/store/stats/RequestCounter.java
@@ -35,7 +35,7 @@ public RequestCounter(int durationMS) {
         this.time = time;
         this.values = new AtomicReference<Accumulator>(new Accumulator());
         this.durationMS = durationMS;
-        this.histogram = new Histogram(15000, 1);
+        this.histogram = new Histogram(65535, 1);
         this.q95LatencyMs = 0;
         this.q99LatencyMs = 0;
     }
@@ -93,13 +93,11 @@ public long getMaxLatencyInMs() {
     private void maybeResetHistogram() {
         Accumulator accum = values.get();
         long now = time.getMilliseconds();
-        if(now - accum.startTimeMS <= durationMS) {
+        if(now - accum.startTimeMS > durationMS) {
             // Reset the histogram            
-            synchronized(histogram) {
-                q95LatencyMs = histogram.getQuantile(0.95);
-                q99LatencyMs = histogram.getQuantile(0.99);
-                histogram.reset();
-            }
+            q95LatencyMs = histogram.getQuantile(0.95);
+            q99LatencyMs = histogram.getQuantile(0.99);
+            histogram.reset();
         }
     }
 
@@ -150,10 +148,8 @@ public void addRequest(long timeNS) {
      * @param getAllAggregatedCount Total number of keys returned for getAll calls
      */
     public void addRequest(long timeNS, long numEmptyResponses, long bytes, long getAllAggregatedCount) {
-        synchronized(histogram) {
-            int timeMs = (int) timeNS / (int) Time.NS_PER_MS;
-            histogram.insert(timeMs);
-        }
+        int timeMs = (int) timeNS / (int) Time.NS_PER_MS;
+        histogram.insert(timeMs);
         maybeResetHistogram();
         for(int i = 0; i < 3; i++) {
             Accumulator oldv = getValidAccumulator();

diff --git a/test/unit/voldemort/store/stats/HistogramTest.java b/test/unit/voldemort/store/stats/HistogramTest.java
@@ -22,11 +22,17 @@ public void setUp() {
         histogram.insert(36);
         histogram.insert(41);
         histogram.insert(46);
+        histogram.insert(56);
+
+        // Test that exceeding the size of the structure merely increments
+        // the last bucket
+        histogram.insert(66);
+        histogram.insert(76);
     }
 
     @Test
     public void testAverage() {
-        assertEquals(histogram.getQuantile(0.50), 20);
+        assertEquals(histogram.getQuantile(0.50), 30);
     }
 
     @Test
@@ -36,7 +42,7 @@ public void test95thQuartile() {
 
     @Test
     public void test99thQuartile() {
-        assertEquals(histogram.getQuantile(0.95), 45);
+        assertEquals(histogram.getQuantile(0.99), 45);
     }
 
 }