geode-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From u..@apache.org
Subject [09/33] incubator-geode git commit: GEODE-1494: Changing stats list to be a CopyOnWriteArrayList
Date Wed, 15 Jun 2016 16:17:42 GMT
GEODE-1494: Changing stats list to be a CopyOnWriteArrayList

The statsList used to track Statistics in InternalDistributedSystem was
managed under a lock. That was problematic, because code that iterates
on the list needs to hold the lock. If the code invoking callbacks for
GEODE-1494 was slow, that means the lock would block other stats
operations.

Changing this list to a copy on write array list to holding a lock for a
long period of time.


Project: http://git-wip-us.apache.org/repos/asf/incubator-geode/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-geode/commit/e9ffdce4
Tree: http://git-wip-us.apache.org/repos/asf/incubator-geode/tree/e9ffdce4
Diff: http://git-wip-us.apache.org/repos/asf/incubator-geode/diff/e9ffdce4

Branch: refs/heads/feature/GEODE-420
Commit: e9ffdce4fdf4f5c3b9f4ae45e19168e3de55ebe9
Parents: 5e0050d
Author: Dan Smith <upthewaterspout@apache.org>
Authored: Tue Jun 7 11:44:24 2016 -0700
Committer: Dan Smith <upthewaterspout@apache.org>
Committed: Mon Jun 13 10:38:36 2016 -0700

----------------------------------------------------------------------
 .../internal/InternalDistributedSystem.java     | 36 ++++-------
 .../internal/AbstractStatisticsFactory.java     | 65 ++++++++------------
 .../gemfire/internal/GemFireStatSampler.java    | 14 ++---
 .../gemfire/internal/HostStatSampler.java       | 10 ++-
 .../cache/control/HeapMemoryMonitor.java        | 20 +++---
 5 files changed, 59 insertions(+), 86 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/e9ffdce4/geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/InternalDistributedSystem.java
----------------------------------------------------------------------
diff --git a/geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/InternalDistributedSystem.java
b/geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/InternalDistributedSystem.java
index 67f5478..a672127 100755
--- a/geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/InternalDistributedSystem.java
+++ b/geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/InternalDistributedSystem.java
@@ -1668,7 +1668,7 @@ public class InternalDistributedSystem
     return sb.toString().trim();
   }
 
-  private final ArrayList<Statistics> statsList = new ArrayList<Statistics>();
+  private final CopyOnWriteArrayList<Statistics> statsList = new CopyOnWriteArrayList<Statistics>();
   private int statsListModCount = 0;
   private long statsListUniqueId = 1;
   private final Object statsListUniqueIdLock = new Object();
@@ -1698,11 +1698,9 @@ public class InternalDistributedSystem
   @Override
   public final Statistics findStatistics(long id) {
     List<Statistics> statsList = this.statsList;
-    synchronized (statsList) {
-      for (Statistics s : statsList) {
-        if (s.getUniqueId() == id) {
-          return s;
-        }
+    for (Statistics s : statsList) {
+      if (s.getUniqueId() == id) {
+        return s;
       }
     }
     throw new RuntimeException(LocalizedStrings.PureStatSampler_COULD_NOT_FIND_STATISTICS_INSTANCE.toLocalizedString());
@@ -1711,11 +1709,9 @@ public class InternalDistributedSystem
   @Override
   public final boolean statisticsExists(long id) {
     List<Statistics> statsList = this.statsList;
-    synchronized (statsList) {
-      for (Statistics s : statsList) {
-        if (s.getUniqueId() == id) {
-          return true;
-        }
+    for (Statistics s : statsList) {
+      if (s.getUniqueId() == id) {
+        return true;
       }
     }
     return false;
@@ -1724,9 +1720,7 @@ public class InternalDistributedSystem
   @Override
   public final Statistics[] getStatistics() {
     List<Statistics> statsList = this.statsList;
-    synchronized (statsList) {
-      return (Statistics[])statsList.toArray(new Statistics[statsList.size()]);
-    }
+    return (Statistics[])statsList.toArray(new Statistics[statsList.size()]);
   }
   
   // StatisticsFactory methods
@@ -1786,10 +1780,8 @@ public class InternalDistributedSystem
    * This method was added to fix bug 40358
    */
   public void visitStatistics(StatisticsVisitor visitor) {
-    synchronized (this.statsList) {
-      for (Statistics s: this.statsList) {
-        visitor.visit(s);
-      }
+    for (Statistics s: this.statsList) {
+      visitor.visit(s);
     }
   }
 
@@ -1844,11 +1836,9 @@ public class InternalDistributedSystem
     return (Statistics[])hits.toArray(result);
   }
   public Statistics findStatisticsByUniqueId(final long uniqueId) {
-    synchronized (this.statsList) {
-      for (Statistics s: this.statsList) {
-        if (uniqueId == s.getUniqueId()) {
-          return s;
-        }
+    for (Statistics s: this.statsList) {
+      if (uniqueId == s.getUniqueId()) {
+        return s;
       }
     }
     return null;

http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/e9ffdce4/geode-core/src/main/java/com/gemstone/gemfire/internal/AbstractStatisticsFactory.java
----------------------------------------------------------------------
diff --git a/geode-core/src/main/java/com/gemstone/gemfire/internal/AbstractStatisticsFactory.java
b/geode-core/src/main/java/com/gemstone/gemfire/internal/AbstractStatisticsFactory.java
index 886258b..972e670 100755
--- a/geode-core/src/main/java/com/gemstone/gemfire/internal/AbstractStatisticsFactory.java
+++ b/geode-core/src/main/java/com/gemstone/gemfire/internal/AbstractStatisticsFactory.java
@@ -28,6 +28,7 @@ import java.io.Reader;
 import java.util.ArrayList;
 import java.util.Iterator;
 import java.util.List;
+import java.util.concurrent.CopyOnWriteArrayList;
 
 /**
  * An abstract standalone implementation of {@link StatisticsFactory}.
@@ -41,7 +42,7 @@ public abstract class AbstractStatisticsFactory
 
   private final long id;
   private final String name;
-  private final List<Statistics> statsList;
+  private final CopyOnWriteArrayList<Statistics> statsList;
   private int statsListModCount = 0;
   private long statsListUniqueId = 1;
   private final Object statsListUniqueIdLock;
@@ -53,7 +54,7 @@ public abstract class AbstractStatisticsFactory
     this.name = name;
     this.startTime = startTime;
     
-    this.statsList = new ArrayList<Statistics>();
+    this.statsList = new CopyOnWriteArrayList<Statistics>();
     this.statsListUniqueIdLock = new Object();
     this.tf = StatisticsTypeFactoryImpl.singleton();
   }
@@ -112,11 +113,9 @@ public abstract class AbstractStatisticsFactory
   @Override
   public final boolean statisticsExists(long id) {
     List<Statistics> statsList = this.statsList;
-    synchronized (statsList) {
-      for (Statistics s : statsList) {
-        if (s.getUniqueId() == id) {
-          return true;
-        }
+    for (Statistics s : statsList) {
+      if (s.getUniqueId() == id) {
+        return true;
       }
     }
     return false;
@@ -125,9 +124,7 @@ public abstract class AbstractStatisticsFactory
   @Override
   public final Statistics[] getStatistics() {
     List<Statistics> statsList = this.statsList;
-    synchronized (statsList) {
-      return (Statistics[])statsList.toArray(new Statistics[statsList.size()]);
-    }
+    return (Statistics[])statsList.toArray(new Statistics[statsList.size()]);
   }
   
   // StatisticsFactory methods
@@ -163,13 +160,11 @@ public abstract class AbstractStatisticsFactory
   @Override
   public final Statistics[] findStatisticsByType(StatisticsType type) {
     List<Statistics> hits = new ArrayList<Statistics>();
-    synchronized (statsList) {
-      Iterator<Statistics> it = statsList.iterator();
-      while (it.hasNext()) {
-        Statistics s = (Statistics)it.next();
-        if (type == s.getType()) {
-          hits.add(s);
-        }
+    Iterator<Statistics> it = statsList.iterator();
+    while (it.hasNext()) {
+      Statistics s = (Statistics)it.next();
+      if (type == s.getType()) {
+        hits.add(s);
       }
     }
     Statistics[] result = new Statistics[hits.size()];
@@ -179,13 +174,11 @@ public abstract class AbstractStatisticsFactory
   @Override
   public final Statistics[] findStatisticsByTextId(String textId) {
     List<Statistics> hits = new ArrayList<Statistics>();
-    synchronized (statsList) {
-      Iterator<Statistics> it = statsList.iterator();
-      while (it.hasNext()) {
-        Statistics s = (Statistics)it.next();
-        if (s.getTextId().equals(textId)) {
-          hits.add(s);
-        }
+    Iterator<Statistics> it = statsList.iterator();
+    while (it.hasNext()) {
+      Statistics s = (Statistics)it.next();
+      if (s.getTextId().equals(textId)) {
+        hits.add(s);
       }
     }
     Statistics[] result = new Statistics[hits.size()];
@@ -195,13 +188,11 @@ public abstract class AbstractStatisticsFactory
   @Override
   public final Statistics[] findStatisticsByNumericId(long numericId) {
     List<Statistics> hits = new ArrayList<Statistics>();
-    synchronized (statsList) {
-      Iterator<Statistics> it = statsList.iterator();
-      while (it.hasNext()) {
-        Statistics s = (Statistics)it.next();
-        if (numericId == s.getNumericId()) {
-          hits.add(s);
-        }
+    Iterator<Statistics> it = statsList.iterator();
+    while (it.hasNext()) {
+      Statistics s = (Statistics)it.next();
+      if (numericId == s.getNumericId()) {
+        hits.add(s);
       }
     }
     Statistics[] result = new Statistics[hits.size()];
@@ -209,13 +200,11 @@ public abstract class AbstractStatisticsFactory
   }
   
   public final Statistics findStatisticsByUniqueId(long uniqueId) {
-    synchronized (statsList) {
-      Iterator<Statistics> it = statsList.iterator();
-      while (it.hasNext()) {
-        Statistics s = (Statistics)it.next();
-        if (uniqueId == s.getUniqueId()) {
-          return s;
-        }
+    Iterator<Statistics> it = statsList.iterator();
+    while (it.hasNext()) {
+      Statistics s = (Statistics)it.next();
+      if (uniqueId == s.getUniqueId()) {
+        return s;
       }
     }
     return null;

http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/e9ffdce4/geode-core/src/main/java/com/gemstone/gemfire/internal/GemFireStatSampler.java
----------------------------------------------------------------------
diff --git a/geode-core/src/main/java/com/gemstone/gemfire/internal/GemFireStatSampler.java
b/geode-core/src/main/java/com/gemstone/gemfire/internal/GemFireStatSampler.java
index 5930506..1eb35d0 100644
--- a/geode-core/src/main/java/com/gemstone/gemfire/internal/GemFireStatSampler.java
+++ b/geode-core/src/main/java/com/gemstone/gemfire/internal/GemFireStatSampler.java
@@ -276,14 +276,12 @@ public final class GemFireStatSampler extends HostStatSampler  {
     }
     if (stopRequested()) return;
     HostStatHelper.readyRefreshOSStats();
-    synchronized (l) {
-      Iterator<Statistics> it = l.iterator();
-      while (it.hasNext()) {
-        if (stopRequested()) return;
-        StatisticsImpl s = (StatisticsImpl)it.next();
-        if (s.usesSystemCalls()) {
-          HostStatHelper.refresh((LocalStatisticsImpl)s);
-        }
+    Iterator<Statistics> it = l.iterator();
+    while (it.hasNext()) {
+      if (stopRequested()) return;
+      StatisticsImpl s = (StatisticsImpl)it.next();
+      if (s.usesSystemCalls()) {
+        HostStatHelper.refresh((LocalStatisticsImpl)s);
       }
     }
   }

http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/e9ffdce4/geode-core/src/main/java/com/gemstone/gemfire/internal/HostStatSampler.java
----------------------------------------------------------------------
diff --git a/geode-core/src/main/java/com/gemstone/gemfire/internal/HostStatSampler.java b/geode-core/src/main/java/com/gemstone/gemfire/internal/HostStatSampler.java
index ebb4177..a5a07ca 100644
--- a/geode-core/src/main/java/com/gemstone/gemfire/internal/HostStatSampler.java
+++ b/geode-core/src/main/java/com/gemstone/gemfire/internal/HostStatSampler.java
@@ -510,12 +510,10 @@ public abstract class HostStatSampler
    */
   private void sampleSpecialStats(boolean prepareOnly) {
     List<Statistics> statsList = getStatisticsManager().getStatsList();
-    synchronized (statsList) {
-      for (Statistics s : statsList) {
-        if (stopRequested()) return;
-        if (s instanceof StatisticsImpl) {
-          ((StatisticsImpl)s).prepareForSample();
-        }
+    for (Statistics s : statsList) {
+      if (stopRequested()) return;
+      if (s instanceof StatisticsImpl) {
+        ((StatisticsImpl)s).prepareForSample();
       }
     }
 

http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/e9ffdce4/geode-core/src/main/java/com/gemstone/gemfire/internal/cache/control/HeapMemoryMonitor.java
----------------------------------------------------------------------
diff --git a/geode-core/src/main/java/com/gemstone/gemfire/internal/cache/control/HeapMemoryMonitor.java
b/geode-core/src/main/java/com/gemstone/gemfire/internal/cache/control/HeapMemoryMonitor.java
index 90a26cb..bc83335 100644
--- a/geode-core/src/main/java/com/gemstone/gemfire/internal/cache/control/HeapMemoryMonitor.java
+++ b/geode-core/src/main/java/com/gemstone/gemfire/internal/cache/control/HeapMemoryMonitor.java
@@ -304,18 +304,16 @@ public void stopMonitoring() {
       sampler.waitForInitialization();
       String tenuredPoolName = getTenuredMemoryPoolMXBean().getName();
       List list = this.cache.getDistributedSystem().getStatsList();
-      synchronized (list) {
-        for (Object o : list) {
-          if (o instanceof StatisticsImpl) {
-            StatisticsImpl si = (StatisticsImpl) o;
-            if (si.getTextId().contains(tenuredPoolName) && si.getType().getName().contains("PoolStats"))
{
-              sampler.addLocalStatListener(this.statListener, si, "currentUsedMemory");
-              if (this.cache.getLoggerI18n().fineEnabled()) {
-                this.cache.getLoggerI18n().fine("Registered stat listener for " + si.getTextId());
-              }
-
-              return true;
+      for (Object o : list) {
+        if (o instanceof StatisticsImpl) {
+          StatisticsImpl si = (StatisticsImpl) o;
+          if (si.getTextId().contains(tenuredPoolName) && si.getType().getName().contains("PoolStats"))
{
+            sampler.addLocalStatListener(this.statListener, si, "currentUsedMemory");
+            if (this.cache.getLoggerI18n().fineEnabled()) {
+              this.cache.getLoggerI18n().fine("Registered stat listener for " + si.getTextId());
             }
+
+            return true;
           }
         }
       }


Mime
View raw message