[GEODE-6889] replyWaitMaxTime stat calculation code needs improvement

• Type: Improvement
• Status: Closed
• Priority: Major
• Resolution: Fixed
• Fix Version/s: 1.10.0
• Component/s: core
• Labels: performance
• Assignee: Murtuza Boxwala
• Reporter: Darrel Schneider
• Created: 18/Jun/19 21:28
• Updated: 26/Sep/19 18:04
• Resolved: 21/Jun/19 16:40

Description

The way the replyWaitMaxTime is calculated has some problems:
1. the elapsed time is calculated using System.currentTimeMillis but should instead use getStatTime (it actually uses getStatTime for the begin timestamp but not for the end timestamp).
2. a local atomic should be used to calculate it instead of reading the value from the Statistics instance. Reading is expensive since it uses synchronization.

The following is a prototype fix for #2 I used when performance testing:

diff --git a/geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionStats.java b/geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionStats.java
index 91c47e2495..d591e35570 100644
--- a/geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionStats.java
+++ b/geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionStats.java
@@ -14,6 +14,8 @@
  */
 package org.apache.geode.distributed.internal;
 
+import java.util.concurrent.atomic.AtomicLong;
+
 import org.apache.logging.log4j.Logger;
 
 import org.apache.geode.StatisticDescriptor;
@@ -1602,6 +1604,8 @@ public class DistributionStats implements DMStats {
     return getStatTime();
   }
 
+  private final AtomicLong replyWaitMaxTime = new AtomicLong();
+
   @Override
   public void endReplyWait(long startNanos, long initTime) {
     if (enableClockStats) {
@@ -1610,8 +1614,17 @@ public class DistributionStats implements DMStats {
     }
     if (initTime != 0) {
       long mswait = System.currentTimeMillis() - initTime;
-      if (mswait > getReplyWaitMaxTime()) {
-        stats.setLong(replyWaitMaxTimeId, mswait);
+      boolean done = false;
+      while (!done) {
+        long currentReplyWaitMaxTime = replyWaitMaxTime.get();
+        if (mswait > currentReplyWaitMaxTime) {
+          done = replyWaitMaxTime.compareAndSet(currentReplyWaitMaxTime, mswait);
+          if (done) {
+            stats.setLong(replyWaitMaxTimeId, mswait);
+          }
+        } else {
+          done = true;
+        }
       }
     }
     stats.incInt(replyWaitsInProgressId, -1);

Issue Links

• links to: GitHub Pull Request #3728