Performance enhancements ported from GemFire 8.2.0.x This increases throughput in GMSMembershipManager by avoiding write-locks on the view in some places.
I also found GMSHealthMonitor was mistakenly referencing a CORBA class's "debug" static variable and found an out-of-date comment in AnalyzeSerializablesJUnitTest. Project: http://git-wip-us.apache.org/repos/asf/incubator-geode/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-geode/commit/3f6acdc7 Tree: http://git-wip-us.apache.org/repos/asf/incubator-geode/tree/3f6acdc7 Diff: http://git-wip-us.apache.org/repos/asf/incubator-geode/diff/3f6acdc7 Branch: refs/heads/master Commit: 3f6acdc70fd6454af6b44a719a318e2106d68e46 Parents: 3e20193 Author: Bruce Schuchardt <bschucha...@pivotal.io> Authored: Tue Jul 19 13:31:09 2016 -0700 Committer: Bruce Schuchardt <bschucha...@pivotal.io> Committed: Tue Jul 19 13:31:09 2016 -0700 ---------------------------------------------------------------------- .../membership/gms/fd/GMSHealthMonitor.java | 8 +- .../gms/mgr/GMSMembershipManager.java | 191 ++++++++++--------- .../AnalyzeSerializablesJUnitTest.java | 4 +- 3 files changed, 109 insertions(+), 94 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/3f6acdc7/geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/fd/GMSHealthMonitor.java ---------------------------------------------------------------------- diff --git a/geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/fd/GMSHealthMonitor.java b/geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/fd/GMSHealthMonitor.java old mode 100755 new mode 100644 index e0f22a5..586427f --- a/geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/fd/GMSHealthMonitor.java +++ b/geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/fd/GMSHealthMonitor.java @@ -295,9 +295,7 @@ public class GMSHealthMonitor implements HealthMonitor, MessageHandler { socket.shutdownOutput(); GMSHealthMonitor.this.stats.incFinalCheckResponsesSent(); GMSHealthMonitor.this.stats.incTcpFinalCheckResponsesSent(); - if (debug) { - logger.debug("HealthMonitor: server replied OK."); - } + logger.debug("HealthMonitor: server replied OK."); } else { if (logger.isDebugEnabled()) { logger.debug("HealthMonitor: sending ERROR reply - my UUID is {},{} received is {},{}. My viewID is {} received is {}", @@ -312,9 +310,7 @@ public class GMSHealthMonitor implements HealthMonitor, MessageHandler { socket.shutdownOutput(); GMSHealthMonitor.this.stats.incFinalCheckResponsesSent(); GMSHealthMonitor.this.stats.incTcpFinalCheckResponsesSent(); - if (debug) { - logger.debug("HealthMonitor: server replied ERROR."); - } + logger.debug("HealthMonitor: server replied ERROR."); } } catch (IOException e) { // this is expected if it is a connection-timeout or other failure http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/3f6acdc7/geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/mgr/GMSMembershipManager.java ---------------------------------------------------------------------- diff --git a/geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/mgr/GMSMembershipManager.java b/geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/mgr/GMSMembershipManager.java index b755f6c..e6ca8e1 100644 --- a/geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/mgr/GMSMembershipManager.java +++ b/geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/mgr/GMSMembershipManager.java @@ -60,8 +60,7 @@ import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; -import java.util.concurrent.locks.ReadWriteLock; -import java.util.concurrent.locks.ReentrantReadWriteLock; +import java.util.concurrent.locks.*; public class GMSMembershipManager implements MembershipManager, Manager { @@ -219,7 +218,9 @@ public class GMSMembershipManager implements MembershipManager, Manager * @see #latestView */ protected ReadWriteLock latestViewLock = new ReentrantReadWriteLock(); - + private final Lock latestViewReadLock = latestViewLock.readLock(); + private final Lock latestViewWriteLock = latestViewLock.writeLock(); + /** * This is the listener that accepts our membership events */ @@ -424,7 +425,7 @@ public class GMSMembershipManager implements MembershipManager, Manager // } // We perform the update under a global lock so that other // incoming events will not be lost in terms of our global view. - latestViewLock.writeLock().lock(); + latestViewWriteLock.lock(); try { // first determine the version for multicast message serialization Version version = Version.CURRENT; @@ -597,7 +598,7 @@ public class GMSMembershipManager implements MembershipManager, Manager catch (DistributedSystemDisconnectedException se) { } } finally { - latestViewLock.writeLock().unlock(); + latestViewWriteLock.unlock(); } } @@ -630,7 +631,7 @@ public class GMSMembershipManager implements MembershipManager, Manager services.setShutdownCause(null); services.getCancelCriterion().cancel(null); - latestViewLock.writeLock().lock(); + latestViewWriteLock.lock(); try { try { this.isJoining = true; // added for bug #44373 @@ -668,7 +669,7 @@ public class GMSMembershipManager implements MembershipManager, Manager } } finally { - latestViewLock.writeLock().unlock(); + latestViewWriteLock.unlock(); } } @@ -880,7 +881,7 @@ public class GMSMembershipManager implements MembershipManager, Manager final InternalDistributedMember member = (InternalDistributedMember)dm; boolean warn = false; - latestViewLock.writeLock().lock(); + latestViewWriteLock.lock(); try { // At this point, the join may have been discovered by // other means. @@ -955,7 +956,7 @@ public class GMSMembershipManager implements MembershipManager, Manager latestView = newMembers; } } finally { - latestViewLock.writeLock().unlock(); + latestViewWriteLock.unlock(); } if (warn) { // fix for bug #41538 - deadlock while alerting logger.warn(LocalizedMessage.create( @@ -969,7 +970,7 @@ public class GMSMembershipManager implements MembershipManager, Manager /** starts periodic task to perform cleanup chores such as expire surprise members */ private void startCleanupTimer() { - latestViewLock.writeLock().lock(); + latestViewWriteLock.lock(); try { if (this.cleanupTimer != null) { return; @@ -980,7 +981,7 @@ public class GMSMembershipManager implements MembershipManager, Manager SystemTimer.SystemTimerTask st = new SystemTimer.SystemTimerTask() { @Override public void run2() { - latestViewLock.writeLock().lock(); + latestViewWriteLock.lock(); try { long oldestAllowed = System.currentTimeMillis() - surpriseMemberTimeout; for (Iterator it=surpriseMembers.entrySet().iterator(); it.hasNext(); ) { @@ -996,14 +997,14 @@ public class GMSMembershipManager implements MembershipManager, Manager } } } finally { - latestViewLock.writeLock().unlock(); + latestViewWriteLock.unlock(); } } }; this.cleanupTimer.scheduleAtFixedRate(st, surpriseMemberTimeout, surpriseMemberTimeout/3); } // ds != null && ds.isConnected() } finally { - latestViewLock.writeLock().unlock(); + latestViewWriteLock.unlock(); } } /** @@ -1029,7 +1030,7 @@ public class GMSMembershipManager implements MembershipManager, Manager } public void warnShun(DistributedMember m) { - latestViewLock.writeLock().lock(); + latestViewWriteLock.lock(); try { if (!shunnedMembers.containsKey(m)) { return; // not shunned @@ -1039,7 +1040,7 @@ public class GMSMembershipManager implements MembershipManager, Manager } shunnedAndWarnedMembers.add(m); } finally { - latestViewLock.writeLock().unlock(); + latestViewWriteLock.unlock(); } // issue warning outside of sync since it may cause messaging and we don't // want to hold the view lock while doing that @@ -1063,47 +1064,48 @@ public class GMSMembershipManager implements MembershipManager, Manager InternalDistributedMember m = msg.getSender(); boolean shunned = false; - // First grab the lock: check the sender against our stabilized view. - latestViewLock.writeLock().lock(); - try { - if (isShunned(m)) { - if (msg instanceof StartupMessage) { - endShun(m); - } - else { - // fix for bug 41538 - sick alert listener causes deadlock - // due to view lock being held during messaging - shunned = true; + // If this member is shunned or new, grab the latestViewWriteLock: update the appropriate data structure. + // synchronized (latestViewLock) { + if (isShunnedOrNew(m)) { + latestViewWriteLock.lock(); + try { + if (isShunned(m)) { + if (msg instanceof StartupMessage) { + endShun(m); + } else { + // fix for bug 41538 - sick alert listener causes deadlock + // due to view latestViewReadWriteLock being held during messaging + shunned = true; + } } - } // isShunned - if (!shunned) { - isNew = !latestView.contains(m) && !surpriseMembers.containsKey(m); - - // If it's a new sender, wait our turn, generate the event - if (isNew) { - shunned = !addSurpriseMember(m); - } // isNew + if (!shunned) { + // If it's a new sender, wait our turn, generate the event + if (isNew(m)) { + shunned = !addSurpriseMember(m); + } + } + } finally { + latestViewWriteLock.unlock(); } - - // Latch the view before we unlock - } finally { - latestViewLock.writeLock().unlock(); } - + if (shunned) { // bug #41538 - shun notification must be outside synchronization to avoid hanging warnShun(m); - logger.info("Membership: Ignoring message from shunned member <{}>:{}", m, msg); + if (logger.isTraceEnabled(LogMarker.DISTRIBUTION_VIEWS)) { + logger.trace(LogMarker.DISTRIBUTION_VIEWS, "Membership: Ignoring message from shunned member <{}>:{}", m, msg); + } throw new MemberShunnedException(m); } - + listener.messageReceived(msg); } - /** - * Process a new view object, or place on the startup queue - * @param viewArg the new view - */ + + /** + * Process a new view object, or place on the startup queue + * @param viewArg the new view + */ protected void handleOrDeferViewEvent(NetView viewArg) { if (this.isJoining) { // bug #44373 - queue all view messages while joining. @@ -1114,7 +1116,7 @@ public class GMSMembershipManager implements MembershipManager, Manager return; } } - latestViewLock.writeLock().lock(); + latestViewWriteLock.lock(); try { synchronized(startupLock) { if (!processingEvents) { @@ -1131,7 +1133,7 @@ public class GMSMembershipManager implements MembershipManager, Manager listener.messageReceived(v); } finally { - latestViewLock.writeLock().unlock(); + latestViewWriteLock.unlock(); } } @@ -1146,7 +1148,7 @@ public class GMSMembershipManager implements MembershipManager, Manager * @param suspectInfo the suspectee and suspector */ protected void handleOrDeferSuspect(SuspectMember suspectInfo) { - latestViewLock.writeLock().lock(); + latestViewWriteLock.lock(); try { synchronized(startupLock) { if (!processingEvents) { @@ -1163,7 +1165,7 @@ public class GMSMembershipManager implements MembershipManager, Manager // let's not get huffy about it } } finally { - latestViewLock.writeLock().unlock(); + latestViewWriteLock.unlock(); } } @@ -1325,9 +1327,9 @@ public class GMSMembershipManager implements MembershipManager, Manager // Grab the latest view under a mutex... NetView v; - latestViewLock.readLock().lock(); + latestViewReadLock.lock(); v = latestView; - latestViewLock.readLock().unlock(); + latestViewReadLock.unlock(); NetView result = new NetView(v, v.getViewId()); @@ -1348,11 +1350,11 @@ public class GMSMembershipManager implements MembershipManager, Manager * @return the lead member associated with the latest view */ public DistributedMember getLeadMember() { - latestViewLock.readLock().lock(); + latestViewReadLock.lock(); try { return latestView == null? null : latestView.getLeadMember(); } finally { - latestViewLock.readLock().unlock(); + latestViewReadLock.unlock(); } } @@ -1365,18 +1367,18 @@ public class GMSMembershipManager implements MembershipManager, Manager * @return the current membership view coordinator */ public DistributedMember getCoordinator() { - latestViewLock.readLock().lock(); + latestViewReadLock.lock(); try { return latestView == null? null : latestView.getCoordinator(); } finally { - latestViewLock.readLock().unlock(); + latestViewReadLock.unlock(); } } public boolean memberExists(DistributedMember m) { - latestViewLock.readLock().lock(); + latestViewReadLock.lock(); NetView v = latestView; - latestViewLock.readLock().unlock(); + latestViewReadLock.unlock(); return v.getMembers().contains(m); } @@ -1500,11 +1502,11 @@ public class GMSMembershipManager implements MembershipManager, Manager // Make sure that channel information is consistent // Probably not important in this particular case, but just // to be consistent... - latestViewLock.writeLock().lock(); + latestViewWriteLock.lock(); try { destroyMember(address, false, "orderly shutdown"); } finally { - latestViewLock.writeLock().unlock(); + latestViewWriteLock.unlock(); } } } @@ -1684,13 +1686,13 @@ public class GMSMembershipManager implements MembershipManager, Manager InternalDistributedMember[] keys; if (content.forAll()) { allDestinations = true; - latestViewLock.writeLock().lock(); + latestViewReadLock.lock(); try { List<InternalDistributedMember> keySet = latestView.getMembers(); keys = new InternalDistributedMember[keySet.size()]; keys = (InternalDistributedMember[])keySet.toArray(keys); } finally { - latestViewLock.writeLock().unlock(); + latestViewReadLock.unlock(); } } else { @@ -1917,11 +1919,11 @@ public class GMSMembershipManager implements MembershipManager, Manager if (m != null) { GMSMember id = (GMSMember)m.getNetMember(); if (!id.hasUUID()) { - latestViewLock.readLock().lock(); + latestViewReadLock.lock(); try { addresses[i] = latestView.getCanonicalID(addresses[i]); } finally { - latestViewLock.readLock().unlock(); + latestViewReadLock.unlock(); } } } @@ -1941,9 +1943,9 @@ public class GMSMembershipManager implements MembershipManager, Manager public void setShutdown() { - latestViewLock.writeLock().lock(); + latestViewWriteLock.lock(); shutdownInProgress = true; - latestViewLock.writeLock().unlock(); + latestViewWriteLock.unlock(); } @Override @@ -1965,7 +1967,7 @@ public class GMSMembershipManager implements MembershipManager, Manager boolean crashed, final String reason) { // Make sure it is removed from the view - latestViewLock.writeLock().lock(); + latestViewWriteLock.lock(); try { if (latestView.contains(member)) { NetView newView = new NetView(latestView, latestView.getViewId()); @@ -1973,7 +1975,7 @@ public class GMSMembershipManager implements MembershipManager, Manager latestView = newView; } } finally { - latestViewLock.writeLock().unlock(); + latestViewWriteLock.unlock(); } surpriseMembers.remove(member); @@ -2031,29 +2033,46 @@ public class GMSMembershipManager implements MembershipManager, Manager * list if it was shunned too far in the past. * * Concurrency: protected by {@link #latestViewLock} ReentrantReadWriteLock - * + * + * @guarded.By latestViewLock * @return true if the given member is a zombie */ public boolean isShunned(DistributedMember m) { - latestViewLock.writeLock().lock(); + if (!shunnedMembers.containsKey(m)) { + return false; + } + + latestViewWriteLock.lock(); try { - if (!shunnedMembers.containsKey(m)) - return false; - // Make sure that the entry isn't stale... long shunTime = ((Long)shunnedMembers.get(m)).longValue(); long now = System.currentTimeMillis(); - if (shunTime + SHUNNED_SUNSET * 1000 > now) + if (shunTime + SHUNNED_SUNSET * 1000 > now) { return true; + } // Oh, it _is_ stale. Remove it while we're here. endShun(m); return false; } finally { - latestViewLock.writeLock().unlock(); + latestViewWriteLock.unlock(); } } + private final boolean isShunnedOrNew(final InternalDistributedMember m) { + latestViewReadLock.lock(); + try { + return shunnedMembers.containsKey(m) || isNew(m); + } finally { // synchronized + latestViewReadLock.unlock(); + } + } + + // must be invoked under view read or write lock + private final boolean isNew(final InternalDistributedMember m) { + return !latestView.contains(m) && !surpriseMembers.containsKey(m); + } + /** * Indicate whether the given member is in the surprise member list * <P> @@ -2068,7 +2087,7 @@ public class GMSMembershipManager implements MembershipManager, Manager * @return true if the given member is a surprise member */ public boolean isSurpriseMember(DistributedMember m) { - latestViewLock.readLock().lock(); + latestViewReadLock.lock(); try { if (surpriseMembers.containsKey(m)) { long birthTime = ((Long)surpriseMembers.get(m)).longValue(); @@ -2077,7 +2096,7 @@ public class GMSMembershipManager implements MembershipManager, Manager } return false; } finally { - latestViewLock.readLock().unlock(); + latestViewReadLock.unlock(); } } @@ -2091,11 +2110,11 @@ public class GMSMembershipManager implements MembershipManager, Manager if (logger.isDebugEnabled()) { logger.debug("test hook is adding surprise member {} birthTime={}", m, birthTime); } - latestViewLock.writeLock().lock(); + latestViewWriteLock.lock(); try { surpriseMembers.put((InternalDistributedMember)m, Long.valueOf(birthTime)); } finally { - latestViewLock.writeLock().unlock(); + latestViewWriteLock.unlock(); } } @@ -2253,11 +2272,11 @@ public class GMSMembershipManager implements MembershipManager, Manager } } if (!wait) { - latestViewLock.readLock().lock(); + latestViewReadLock.lock(); try { wait = this.latestView.contains(idm); } finally { - latestViewLock.readLock().unlock(); + latestViewReadLock.unlock(); } if (wait && logger.isDebugEnabled()) { logger.debug("waiting for {} to leave the membership view", mbr); @@ -2310,7 +2329,7 @@ public class GMSMembershipManager implements MembershipManager, Manager CountDownLatch currentLatch = null; // ARB: preconditions // remoteId != null - latestViewLock.writeLock().lock(); + latestViewWriteLock.lock(); try { if (latestView == null) { // Not sure how this would happen, but see bug 38460. @@ -2326,7 +2345,7 @@ public class GMSMembershipManager implements MembershipManager, Manager this.memberLatch.put(remoteId, currentLatch); } } finally { - latestViewLock.writeLock().unlock(); + latestViewWriteLock.unlock(); } if (!foundRemoteId) { @@ -2381,7 +2400,7 @@ public class GMSMembershipManager implements MembershipManager, Manager public void registerTestHook(MembershipTestHook mth) { // lock for additions to avoid races during startup - latestViewLock.writeLock().lock(); + latestViewWriteLock.lock(); try { if (this.membershipTestHooks == null) { this.membershipTestHooks = Collections.singletonList(mth); @@ -2392,12 +2411,12 @@ public class GMSMembershipManager implements MembershipManager, Manager this.membershipTestHooks = l; } } finally { - latestViewLock.writeLock().unlock(); + latestViewWriteLock.unlock(); } } public void unregisterTestHook(MembershipTestHook mth) { - latestViewLock.writeLock().lock(); + latestViewWriteLock.lock(); try { if (this.membershipTestHooks != null) { if (this.membershipTestHooks.size() == 1) { @@ -2409,7 +2428,7 @@ public class GMSMembershipManager implements MembershipManager, Manager } } } finally { - latestViewLock.writeLock().unlock(); + latestViewWriteLock.unlock(); } } http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/3f6acdc7/geode-core/src/test/java/com/gemstone/gemfire/codeAnalysis/AnalyzeSerializablesJUnitTest.java ---------------------------------------------------------------------- diff --git a/geode-core/src/test/java/com/gemstone/gemfire/codeAnalysis/AnalyzeSerializablesJUnitTest.java b/geode-core/src/test/java/com/gemstone/gemfire/codeAnalysis/AnalyzeSerializablesJUnitTest.java index 1c9b8c4..d551c9f 100644 --- a/geode-core/src/test/java/com/gemstone/gemfire/codeAnalysis/AnalyzeSerializablesJUnitTest.java +++ b/geode-core/src/test/java/com/gemstone/gemfire/codeAnalysis/AnalyzeSerializablesJUnitTest.java @@ -53,9 +53,9 @@ public class AnalyzeSerializablesJUnitTest { @Before public void loadClasses() throws Exception { String version = System.getProperty("java.runtime.version"); - boolean jdk17 = version != null && version.startsWith("1.8"); + boolean jdk18 = version != null && version.startsWith("1.8"); // sanctioned info is based on a 1.7 compiler - Assume.assumeTrue("AnalyzeSerializables requires a Java 7 but tests are running with v"+version, jdk17); + Assume.assumeTrue("AnalyzeSerializables requires a Java 8 but tests are running with v"+version, jdk18); if (classes.size() > 0) { return; }
