From fa8a013594c4252dd61bd74d2fea0d411334e25d Mon Sep 17 00:00:00 2001 From: Jacob Barrett Date: Mon, 18 Feb 2019 09:17:18 -0800 Subject: [PATCH 1/5] GEODE-6424: Greatly improves statistic counter storage throughput. Reduces thread contention by using LongAdder and DoubleAdder to store counters. Benchmarking (on specific hardware) showed Atomic50StatisticsImpl could perform about 41M increments/second regardless of the number of threads updating the counter. Poor use of volatile memory access and CAS operations created unnecessary contention. The replacement, StripedStatisticsImpl, uses LongAdder and DoubleAdder to reduce contention. The same benchmark showed the throughput in increments/second scale nearly linearly up to the physical hardware threads of the host, seeing values as high as 2.8B increments/second on a 36 thread host. --- build.gradle | 2 +- .../geode/internal/concurrent/Atomics.java | 9 +- .../statistics/StripedStatisticsImpl.java | 88 ++++ .../stats50/Atomic50StatisticsImpl.java | 483 ------------------ .../internal/cache/CachePerfStatsTest.java | 53 +- 5 files changed, 119 insertions(+), 516 deletions(-) create mode 100644 geode-core/src/main/java/org/apache/geode/internal/statistics/StripedStatisticsImpl.java delete mode 100644 geode-core/src/main/java/org/apache/geode/internal/stats50/Atomic50StatisticsImpl.java diff --git a/build.gradle b/build.gradle index 2b1460453ba2..7b30b71e042d 100755 --- a/build.gradle +++ b/build.gradle @@ -34,7 +34,7 @@ buildscript { classpath "gradle.plugin.org.nosphere.apache:creadur-rat-gradle:0.3.1" classpath 'org.sonarsource.scanner.gradle:sonarqube-gradle-plugin:2.6.2' classpath "com.diffplug.spotless:spotless-plugin-gradle:3.10.0" - classpath "me.champeau.gradle:jmh-gradle-plugin:0.4.7" + classpath "me.champeau.gradle:jmh-gradle-plugin:0.4.8" classpath 'com.github.ben-manes:gradle-versions-plugin:0.17.0' classpath 'io.spring.gradle:dependency-management-plugin:1.0.6.RELEASE' } diff --git a/geode-core/src/main/java/org/apache/geode/internal/concurrent/Atomics.java b/geode-core/src/main/java/org/apache/geode/internal/concurrent/Atomics.java index 3e9c036dbca8..193210a95a27 100644 --- a/geode-core/src/main/java/org/apache/geode/internal/concurrent/Atomics.java +++ b/geode-core/src/main/java/org/apache/geode/internal/concurrent/Atomics.java @@ -22,8 +22,7 @@ import org.apache.geode.distributed.internal.DistributionConfig; import org.apache.geode.internal.statistics.LocalStatisticsImpl; import org.apache.geode.internal.statistics.StatisticsManager; -import org.apache.geode.internal.statistics.StatisticsTypeImpl; -import org.apache.geode.internal.stats50.Atomic50StatisticsImpl; +import org.apache.geode.internal.statistics.StripedStatisticsImpl; public class Atomics { private Atomics() {} @@ -39,10 +38,10 @@ private Atomics() {} public static Statistics createAtomicStatistics(StatisticsType type, String textId, long nId, long uId, StatisticsManager mgr) { Statistics result = null; - if (((StatisticsTypeImpl) type).getDoubleStatCount() == 0 && !STRIPED_STATS_DISABLED) { - result = new Atomic50StatisticsImpl(type, textId, nId, uId, mgr); - } else { + if (STRIPED_STATS_DISABLED) { result = new LocalStatisticsImpl(type, textId, nId, uId, true, 0, mgr); + } else { + result = new StripedStatisticsImpl(type, textId, nId, uId); } return result; } diff --git a/geode-core/src/main/java/org/apache/geode/internal/statistics/StripedStatisticsImpl.java b/geode-core/src/main/java/org/apache/geode/internal/statistics/StripedStatisticsImpl.java new file mode 100644 index 000000000000..246b31b915c9 --- /dev/null +++ b/geode-core/src/main/java/org/apache/geode/internal/statistics/StripedStatisticsImpl.java @@ -0,0 +1,88 @@ +package org.apache.geode.internal.statistics; + +import java.util.concurrent.atomic.DoubleAdder; +import java.util.concurrent.atomic.LongAdder; +import java.util.stream.Stream; + +import org.apache.geode.StatisticsType; + +/** + * Stripes statistic counters across threads to reduce contention using {@link LongAdder} and + * {@link DoubleAdder}. + */ +public class StripedStatisticsImpl extends StatisticsImpl { + + private final LongAdder[] intAdders; + private final LongAdder[] longAdders; + private final DoubleAdder[] doubleAdders; + + public StripedStatisticsImpl(StatisticsType type, String textId, long numericId, + long uniqueId) { + super(type, textId, numericId, uniqueId, 0); + + StatisticsTypeImpl realType = (StatisticsTypeImpl) type; + + this.intAdders = + Stream.generate(LongAdder::new).limit(realType.getIntStatCount()).toArray(LongAdder[]::new); + this.longAdders = + Stream.generate(LongAdder::new).limit(realType.getLongStatCount()) + .toArray(LongAdder[]::new); + this.doubleAdders = + Stream.generate(DoubleAdder::new).limit(realType.getDoubleStatCount()) + .toArray(DoubleAdder[]::new); + } + + + @Override + public boolean isAtomic() { + return true; + } + + @Override + protected void _setInt(int offset, int value) { + intAdders[offset].reset(); + intAdders[offset].add(value); + } + + @Override + protected void _setLong(int offset, long value) { + longAdders[offset].reset(); + longAdders[offset].add(value); + } + + @Override + protected void _setDouble(int offset, double value) { + doubleAdders[offset].reset(); + doubleAdders[offset].add(value); + } + + @Override + protected int _getInt(int offset) { + return intAdders[offset].intValue(); + } + + @Override + protected long _getLong(int offset) { + return longAdders[offset].sum(); + } + + @Override + protected double _getDouble(int offset) { + return doubleAdders[offset].sum(); + } + + @Override + protected void _incInt(int offset, int delta) { + intAdders[offset].add(delta); + } + + @Override + protected void _incLong(int offset, long delta) { + longAdders[offset].add(delta); + } + + @Override + protected void _incDouble(int offset, double delta) { + doubleAdders[offset].add(delta); + } +} diff --git a/geode-core/src/main/java/org/apache/geode/internal/stats50/Atomic50StatisticsImpl.java b/geode-core/src/main/java/org/apache/geode/internal/stats50/Atomic50StatisticsImpl.java deleted file mode 100644 index b2f8717d3cbf..000000000000 --- a/geode-core/src/main/java/org/apache/geode/internal/stats50/Atomic50StatisticsImpl.java +++ /dev/null @@ -1,483 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one or more contributor license - * agreements. See the NOTICE file distributed with this work for additional information regarding - * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance with the License. You may obtain a - * copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software distributed under the License - * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express - * or implied. See the License for the specific language governing permissions and limitations under - * the License. - */ -package org.apache.geode.internal.stats50; - -import java.util.ArrayList; -import java.util.concurrent.ConcurrentLinkedQueue; -import java.util.concurrent.CopyOnWriteArrayList; -import java.util.concurrent.atomic.AtomicIntegerArray; -import java.util.concurrent.atomic.AtomicLongArray; - -import org.apache.geode.Statistics; -import org.apache.geode.StatisticsType; -import org.apache.geode.internal.statistics.StatisticsImpl; -import org.apache.geode.internal.statistics.StatisticsManager; -import org.apache.geode.internal.statistics.StatisticsTypeImpl; - -/** - * An implementation of {@link Statistics} that stores its statistics in local java memory. - * - * @see Package introduction - * - * - * @since GemFire 3.0 - * - */ -public class Atomic50StatisticsImpl extends StatisticsImpl { - - /** In JOM Statistics, the values of the int statistics */ - private final AtomicIntegerArray intStorage; - private final AtomicIntegerArray intDirty; - private final Object[] intReadPrepLock; - - /** In JOM Statistics, the values of the long statistics */ - private final AtomicLongArray longStorage; - private final AtomicIntegerArray longDirty; - private final Object[] longReadPrepLock; - - /** The statistics manager that created this instance */ - private final StatisticsManager statisticsManager; - - /////////////////////// Constructors /////////////////////// - - /** - * Creates a new statistics instance of the given type - * - * @param type A description of the statistics - * @param textId Text that identifies this statistic when it is monitored - * @param numericId A number that displayed when this statistic is monitored - * @param uniqueId A number that uniquely identifies this instance - * @param statisticsManager The statistics manager that is creating this instance - */ - public Atomic50StatisticsImpl(StatisticsType type, String textId, long numericId, long uniqueId, - StatisticsManager statisticsManager) { - super(type, textId, numericId, uniqueId, 0); - this.statisticsManager = statisticsManager; - - StatisticsTypeImpl realType = (StatisticsTypeImpl) type; - if (realType.getDoubleStatCount() > 0) { - throw new IllegalArgumentException( - "Atomics do not support double stats"); - } - int intCount = realType.getIntStatCount(); - int longCount = realType.getLongStatCount(); - - if (intCount > 0) { - this.intStorage = new AtomicIntegerArray(intCount); - this.intDirty = new AtomicIntegerArray(intCount); - this.intReadPrepLock = new Object[intCount]; - for (int i = 0; i < intCount; i++) { - this.intReadPrepLock[i] = new Object(); - } - } else { - this.intStorage = null; - this.intDirty = null; - this.intReadPrepLock = null; - } - - if (longCount > 0) { - this.longStorage = new AtomicLongArray(longCount); - this.longDirty = new AtomicIntegerArray(longCount); - this.longReadPrepLock = new Object[longCount]; - for (int i = 0; i < longCount; i++) { - this.longReadPrepLock[i] = new Object(); - } - } else { - this.longStorage = null; - this.longDirty = null; - this.longReadPrepLock = null; - } - } - - ////////////////////// Instance Methods ////////////////////// - - @Override - public boolean isAtomic() { - return true; - } - - @Override - public void close() { - super.close(); - if (this.statisticsManager != null) { - statisticsManager.destroyStatistics(this); - } - } - - /** - * Queue of new ThreadStorage instances. - */ - private ConcurrentLinkedQueue threadStoreQ = - new ConcurrentLinkedQueue(); - /** - * List of ThreadStorage instances that will be used to roll up stat values on this instance. They - * come from the threadStoreQ. - */ - private CopyOnWriteArrayList threadStoreList = - new CopyOnWriteArrayList(); - - /** - * The workspace each thread that modifies statistics will use to do the mods locally. - */ - private static class ThreadStorage { - private final Thread owner; - public volatile boolean dirty = false; - public final AtomicIntegerArray intStore; - public final AtomicLongArray longStore; - - public boolean isAlive() { - return this.owner.isAlive(); - } - - public ThreadStorage(int intSize, int longSize) { - this.owner = Thread.currentThread(); - if (intSize > 0) { - this.intStore = new AtomicIntegerArray(intSize); - } else { - this.intStore = null; - } - if (longSize > 0) { - this.longStore = new AtomicLongArray(longSize); - } else { - this.longStore = null; - } - } - } - - private final ThreadLocal threadStore = new ThreadLocal(); - - private ThreadStorage getThreadStorage() { - ThreadStorage result = this.threadStore.get(); - if (result == null) { - int intSize = 0; - int longSize = 0; - if (this.intStorage != null) { - intSize = this.intStorage.length(); - } - if (this.longStorage != null) { - longSize = this.longStorage.length(); - } - result = new ThreadStorage(intSize, longSize); - this.threadStore.set(result); - this.threadStoreQ.add(result); - } - return result; - } - - private ThreadStorage getThreadStorageForWrite() { - ThreadStorage result = getThreadStorage(); - if (!result.dirty) - result.dirty = true; - return result; - } - - private AtomicIntegerArray getThreadIntStorage() { - return getThreadStorageForWrite().intStore; - } - - private AtomicLongArray getThreadLongStorage() { - return getThreadStorageForWrite().longStore; - } - - //////////////////////// store() Methods /////////////////////// - - @Override - protected void _setInt(int offset, int value) { - doIntWrite(offset, value); - } - - @Override - protected void _setLong(int offset, long value) { - doLongWrite(offset, value); - } - - @Override - protected void _setDouble(int offset, double value) { - throw new IllegalStateException( - "double stats not on Atomic50"); - } - - /////////////////////// get() Methods /////////////////////// - - @Override - protected int _getInt(int offset) { - return doIntRead(offset); - } - - @Override - protected long _getLong(int offset) { - return doLongRead(offset); - } - - @Override - protected double _getDouble(int offset) { - throw new IllegalStateException( - "double stats not on Atomic50"); - } - - //////////////////////// inc() Methods //////////////////////// - - @Override - protected void _incInt(int offset, int delta) { - getThreadIntStorage().getAndAdd(offset, delta); - setIntDirty(offset); - } - - @Override - protected void _incLong(int offset, long delta) { - getThreadLongStorage().getAndAdd(offset, delta); - setLongDirty(offset); - } - - @Override - protected void _incDouble(int offset, double delta) { - throw new IllegalStateException( - "double stats not on Atomic50"); - } - - private static final ThreadLocal samplerThread = new ThreadLocal(); - - /** - * Prepare the threadStoreList by moving into it all the new instances in Q. - */ - @edu.umd.cs.findbugs.annotations.SuppressWarnings( - value = "JLM_JSR166_UTILCONCURRENT_MONITORENTER", - justification = "findbugs complains about this synchronize. It could be changed to a sync on a dedicated Object instance to make findbugs happy. see comments below") - private void prepareThreadStoreList() { - // The following sync is for the rare case when this method is called concurrently. - // In that case it would be sub-optimal for both threads to concurrently create their - // own ArrayList and then for both of them to call addAll. - // findbugs complains about this synchronize. It could be changed to a sync on a dedicated - // Object instance to make findbugs happy. - synchronized (threadStoreList) { - ThreadStorage ts = this.threadStoreQ.poll(); - if (ts == null) - return; - ArrayList tmp = new ArrayList(64); - do { - tmp.add(ts); - ts = this.threadStoreQ.poll(); - } while (ts != null); - if (tmp.size() > 0) { - this.threadStoreList.addAll(tmp); - } - } - } - - /** - * Used to take striped thread stats and "roll them up" into a single shared stat. - * - * @since GemFire 5.1 - */ - @Override - public void prepareForSample() { - // mark this thread as the sampler - if (samplerThread.get() == null) - samplerThread.set(Boolean.TRUE); - prepareThreadStoreList(); - ArrayList removed = null; - for (ThreadStorage ts : this.threadStoreList) { - if (!ts.isAlive()) { - if (removed == null) { - removed = new ArrayList(64); - } - removed.add(ts); - } - if (ts.dirty) { - ts.dirty = false; - if (ts.intStore != null) { - for (int i = 0; i < ts.intStore.length(); i++) { - synchronized (this.intReadPrepLock[i]) { - int delta = ts.intStore.getAndSet(i, 0); - if (delta != 0) { - this.intStorage.getAndAdd(i, delta); - } - } - } - } - if (ts.longStore != null) { - for (int i = 0; i < ts.longStore.length(); i++) { - synchronized (this.longReadPrepLock[i]) { - long delta = ts.longStore.getAndSet(i, 0); - if (delta != 0) { - this.longStorage.getAndAdd(i, delta); - } - } - } - } - } - } - if (removed != null) { - this.threadStoreList.removeAll(removed); - } - } - - private boolean isIntDirty(final int idx) { - return this.intDirty.get(idx) != 0; - } - - private boolean isLongDirty(final int idx) { - return this.longDirty.get(idx) != 0; - } - - private boolean clearIntDirty(final int idx) { - if (!this.intDirty.weakCompareAndSet(idx, 1/* expected */, 0/* update */)) { - return this.intDirty.compareAndSet(idx, 1/* expected */, 0/* update */); - } - return true; - } - - private boolean clearLongDirty(final int idx) { - if (!this.longDirty.weakCompareAndSet(idx, 1/* expected */, 0/* update */)) { - return this.longDirty.compareAndSet(idx, 1/* expected */, 0/* update */); - } - return true; - } - - private void setIntDirty(final int idx) { - if (!this.intDirty.weakCompareAndSet(idx, 0/* expected */, 1/* update */)) { - if (!isIntDirty(idx)) { - this.intDirty.set(idx, 1); - } - } - } - - private void setLongDirty(final int idx) { - if (!this.longDirty.weakCompareAndSet(idx, 0/* expected */, 1/* update */)) { - if (!isLongDirty(idx)) { - this.longDirty.set(idx, 1); - } - } - } - - private int doIntRead(final int idx) { - // early out for sampler; it called prepareForSample - if (samplerThread.get() != null) { - return this.intStorage.get(idx); - } - synchronized (this.intReadPrepLock[idx]) { - if (!isIntDirty(idx)) { - // no need to prepare if not dirty - return this.intStorage.get(idx); - } - } - // this can take a while so release sync - prepareThreadStoreList(); - synchronized (this.intReadPrepLock[idx]) { - if (!clearIntDirty(idx)) { - // no need to prepare if not dirty - return this.intStorage.get(idx); - } - int delta = 0; - for (ThreadStorage ts : this.threadStoreList) { - delta += ts.intStore.getAndSet(idx, 0); - } - if (delta != 0) { - return this.intStorage.addAndGet(idx, delta); - } else { - return this.intStorage.get(idx); - } - } - } - - private void doIntWrite(final int idx, int value) { - synchronized (this.intReadPrepLock[idx]) { - if (!isIntDirty(idx)) { - // no need to prepare if not dirty - this.intStorage.set(idx, value); - return; - } - } - prepareThreadStoreList(); - synchronized (this.intReadPrepLock[idx]) { - if (clearIntDirty(idx)) { - for (ThreadStorage ts : this.threadStoreList) { - if (ts.intStore.get(idx) != 0) { - ts.intStore.set(idx, 0); - } - } - } - this.intStorage.set(idx, value); - } - } - - private long doLongRead(final int idx) { - if (samplerThread.get() != null) { - return this.longStorage.get(idx); - } - synchronized (this.longReadPrepLock[idx]) { - if (!isLongDirty(idx)) { - // no need to prepare if not dirty - return this.longStorage.get(idx); - } - } - // this can take a while so release sync - prepareThreadStoreList(); - synchronized (this.longReadPrepLock[idx]) { - if (!clearLongDirty(idx)) { - // no need to prepare if not dirty - return this.longStorage.get(idx); - } - long delta = 0; - for (ThreadStorage ts : this.threadStoreList) { - delta += ts.longStore.getAndSet(idx, 0); - } - if (delta != 0) { - return this.longStorage.addAndGet(idx, delta); - } else { - return this.longStorage.get(idx); - } - } - } - - private void doLongWrite(int idx, long value) { - synchronized (this.longReadPrepLock[idx]) { - if (!isLongDirty(idx)) { - // no need to prepare if not dirty - this.longStorage.set(idx, value); - return; - } - } - // this can take a while so release sync - prepareThreadStoreList(); - synchronized (this.longReadPrepLock[idx]) { - if (clearLongDirty(idx)) { - for (ThreadStorage ts : this.threadStoreList) { - if (ts.longStore.get(idx) != 0) { - ts.longStore.set(idx, 0); - } - } - } - this.longStorage.set(idx, value); - } - } - - /////////////////// internal package methods ////////////////// - - int[] _getIntStorage() { - throw new IllegalStateException( - "direct access not on Atomic50"); - } - - long[] _getLongStorage() { - throw new IllegalStateException( - "direct access not on Atomic50"); - } - - double[] _getDoubleStorage() { - throw new IllegalStateException( - "direct access not on Atomic50"); - } -} diff --git a/geode-core/src/test/java/org/apache/geode/internal/cache/CachePerfStatsTest.java b/geode-core/src/test/java/org/apache/geode/internal/cache/CachePerfStatsTest.java index c17a634071b0..f8c18aa86200 100644 --- a/geode-core/src/test/java/org/apache/geode/internal/cache/CachePerfStatsTest.java +++ b/geode-core/src/test/java/org/apache/geode/internal/cache/CachePerfStatsTest.java @@ -65,7 +65,7 @@ import org.apache.geode.StatisticsType; import org.apache.geode.internal.cache.CachePerfStats.Clock; import org.apache.geode.internal.statistics.StatisticsManager; -import org.apache.geode.internal.stats50.Atomic50StatisticsImpl; +import org.apache.geode.internal.statistics.StripedStatisticsImpl; /** * Unit tests for {@link CachePerfStats}. @@ -86,8 +86,7 @@ public void setUp() { StatisticsFactory statisticsFactory = mock(StatisticsFactory.class); Clock clock = mock(Clock.class); - statistics = spy(new Atomic50StatisticsImpl(statisticsType, TEXT_ID, 1, 1, - statisticsManager)); + statistics = spy(new StripedStatisticsImpl(statisticsType, TEXT_ID, 1, 1)); when(clock.getTime()).thenReturn(CLOCK_TIME); when(statisticsFactory.createAtomicStatistics(eq(statisticsType), eq(TEXT_ID))) @@ -110,8 +109,8 @@ public void getPutsDelegatesToStatistics() { } /** - * Characterization test: Note that the only way to increment {@code puts} is to invoke - * {@code endPut}. + * Characterization test: Note that the only way to increment {@code puts} is to invoke {@code + * endPut}. */ @Test public void endPutIncrementsPuts() { @@ -140,8 +139,8 @@ public void getGetsDelegatesToStatistics() { } /** - * Characterization test: Note that the only way to increment {@code gets} is to invoke - * {@code endGet}. + * Characterization test: Note that the only way to increment {@code gets} is to invoke {@code + * endGet}. */ @Test public void endGetIncrementsGets() { @@ -171,8 +170,8 @@ public void getPutTimeDelegatesToStatistics() { } /** - * Characterization test: Note that the only way to increment {@code putTime} is to invoke - * {@code endPut}. + * Characterization test: Note that the only way to increment {@code putTime} is to invoke {@code + * endPut}. */ @Test public void endPutIncrementsPutTime() { @@ -190,8 +189,8 @@ public void getGetTimeDelegatesToStatistics() { } /** - * Characterization test: Note that the only way to increment {@code getTime} is to invoke - * {@code endGet}. + * Characterization test: Note that the only way to increment {@code getTime} is to invoke {@code + * endGet}. */ @Test public void endGetIncrementsGetTime() { @@ -260,8 +259,8 @@ public void getPutAllsDelegatesToStatistics() { } /** - * Characterization test: Note that the only way to increment {@code putalls} is to invoke - * {@code endPutAll}. + * Characterization test: Note that the only way to increment {@code putalls} is to invoke {@code + * endPutAll}. */ @Test public void endPutAllIncrementsDestroys() { @@ -320,8 +319,8 @@ public void getUpdatesDelegatesToStatistics() { } /** - * Characterization test: Note that the only way to increment {@code updates} is to invoke - * {@code endPut}. + * Characterization test: Note that the only way to increment {@code updates} is to invoke {@code + * endPut}. */ @Test public void endPutIncrementsUpdates() { @@ -376,8 +375,8 @@ public void getMissesDelegatesToStatistics() { } /** - * Characterization test: Note that the only way to increment {@code misses} is to invoke - * {@code endGet}. + * Characterization test: Note that the only way to increment {@code misses} is to invoke {@code + * endGet}. */ @Test public void endGetIncrementsMisses() { @@ -612,8 +611,8 @@ public void getGetInitialImagesCompletedDelegatesToStatistics() { } /** - * Characterization test: Note that the only way to increment {@code getInitialImagesCompleted} - * is to invoke {@code endGetInitialImage}. + * Characterization test: Note that the only way to increment {@code getInitialImagesCompleted} is + * to invoke {@code endGetInitialImage}. */ @Test public void endCacheWriterCallIncrementsGetInitialImagesCompleted() { @@ -969,8 +968,8 @@ public void getDeltaUpdatesDelegatesToStatistics() { } /** - * Characterization test: Note that the only way to increment {@code deltaUpdates} is to - * invoke {@code endDeltaUpdate}. + * Characterization test: Note that the only way to increment {@code deltaUpdates} is to invoke + * {@code endDeltaUpdate}. */ @Test public void endDeltaUpdateIncrementsDeltaUpdates() { @@ -1007,8 +1006,8 @@ public void incDeltaFailedUpdatesIncrementsDeltaFailedUpdates() { } /** - * Characterization test: {@code deltaFailedUpdates} currently wraps to negative from max - * integer value. + * Characterization test: {@code deltaFailedUpdates} currently wraps to negative from max integer + * value. */ @Test public void deltaFailedUpdatesWrapsFromMaxIntegerToNegativeValue() { @@ -1027,8 +1026,8 @@ public void getDeltasPreparedUpdatesDelegatesToStatistics() { } /** - * Characterization test: Note that the only way to increment {@code deltasPrepared} is to - * invoke {@code endDeltaPrepared}. + * Characterization test: Note that the only way to increment {@code deltasPrepared} is to invoke + * {@code endDeltaPrepared}. */ @Test public void endDeltaPreparedIncrementsDeltasPrepared() { @@ -1038,8 +1037,8 @@ public void endDeltaPreparedIncrementsDeltasPrepared() { } /** - * Characterization test: {@code deltasPrepared} currently wraps to negative from max - * integer value. + * Characterization test: {@code deltasPrepared} currently wraps to negative from max integer + * value. */ @Test public void deltasPreparedWrapsFromMaxIntegerToNegativeValue() { From 2a68eddb09799241c0fe747e46348580e483b4cd Mon Sep 17 00:00:00 2001 From: Jacob Barrett Date: Tue, 19 Feb 2019 11:12:06 -0800 Subject: [PATCH 2/5] GEODE-6424: Adds missing license header. --- .../statistics/StripedStatisticsImpl.java | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/geode-core/src/main/java/org/apache/geode/internal/statistics/StripedStatisticsImpl.java b/geode-core/src/main/java/org/apache/geode/internal/statistics/StripedStatisticsImpl.java index 246b31b915c9..aa4ed9a5af67 100644 --- a/geode-core/src/main/java/org/apache/geode/internal/statistics/StripedStatisticsImpl.java +++ b/geode-core/src/main/java/org/apache/geode/internal/statistics/StripedStatisticsImpl.java @@ -1,3 +1,18 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more contributor license + * agreements. See the NOTICE file distributed with this work for additional information regarding + * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the License. You may obtain a + * copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License + * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express + * or implied. See the License for the specific language governing permissions and limitations under + * the License. + */ + package org.apache.geode.internal.statistics; import java.util.concurrent.atomic.DoubleAdder; From 1cbd37d0851af24b1fd772fa8e4ea27ea4389526 Mon Sep 17 00:00:00 2001 From: Jacob Barrett Date: Tue, 19 Feb 2019 11:45:16 -0800 Subject: [PATCH 3/5] GEODE-6424: Fixes stats closing issue. --- .../geode/internal/concurrent/Atomics.java | 2 +- .../internal/statistics/LocalStatisticsImpl.java | 16 +--------------- .../internal/statistics/StatisticsImpl.java | 9 ++++++++- .../statistics/StripedStatisticsImpl.java | 5 ++--- .../geode/internal/cache/CachePerfStatsTest.java | 2 +- .../geode/internal/statistics/StatUtils.java | 2 ++ 6 files changed, 15 insertions(+), 21 deletions(-) diff --git a/geode-core/src/main/java/org/apache/geode/internal/concurrent/Atomics.java b/geode-core/src/main/java/org/apache/geode/internal/concurrent/Atomics.java index 193210a95a27..8ba437d21442 100644 --- a/geode-core/src/main/java/org/apache/geode/internal/concurrent/Atomics.java +++ b/geode-core/src/main/java/org/apache/geode/internal/concurrent/Atomics.java @@ -41,7 +41,7 @@ public static Statistics createAtomicStatistics(StatisticsType type, String text if (STRIPED_STATS_DISABLED) { result = new LocalStatisticsImpl(type, textId, nId, uId, true, 0, mgr); } else { - result = new StripedStatisticsImpl(type, textId, nId, uId); + result = new StripedStatisticsImpl(type, textId, nId, uId, mgr); } return result; } diff --git a/geode-core/src/main/java/org/apache/geode/internal/statistics/LocalStatisticsImpl.java b/geode-core/src/main/java/org/apache/geode/internal/statistics/LocalStatisticsImpl.java index f1396b2fb74e..516736751c1f 100644 --- a/geode-core/src/main/java/org/apache/geode/internal/statistics/LocalStatisticsImpl.java +++ b/geode-core/src/main/java/org/apache/geode/internal/statistics/LocalStatisticsImpl.java @@ -52,9 +52,6 @@ public class LocalStatisticsImpl extends StatisticsImpl { */ private final transient Object[] doubleLocks; - /** The StatisticsFactory that created this instance */ - private final StatisticsManager statisticsManager; - /////////////////////// Constructors /////////////////////// /** @@ -72,10 +69,7 @@ public class LocalStatisticsImpl extends StatisticsImpl { */ public LocalStatisticsImpl(StatisticsType type, String textId, long numericId, long uniqueId, boolean atomicIncrements, int osStatFlags, StatisticsManager statisticsManager) { - super(type, textId, numericId, uniqueId, - osStatFlags); - - this.statisticsManager = statisticsManager; + super(type, textId, numericId, uniqueId, osStatFlags, statisticsManager); StatisticsTypeImpl realType = (StatisticsTypeImpl) type; int intCount = realType.getIntStatCount(); @@ -153,14 +147,6 @@ public boolean isAtomic() { return intLocks != null || longLocks != null || doubleLocks != null; } - @Override - public void close() { - super.close(); - if (this.statisticsManager != null) { - statisticsManager.destroyStatistics(this); - } - } - //////////////////////// store() Methods /////////////////////// @Override diff --git a/geode-core/src/main/java/org/apache/geode/internal/statistics/StatisticsImpl.java b/geode-core/src/main/java/org/apache/geode/internal/statistics/StatisticsImpl.java index 02ae01ecc69e..74c6e3b53c45 100644 --- a/geode-core/src/main/java/org/apache/geode/internal/statistics/StatisticsImpl.java +++ b/geode-core/src/main/java/org/apache/geode/internal/statistics/StatisticsImpl.java @@ -71,6 +71,9 @@ public abstract class StatisticsImpl implements Statistics { /** Uniquely identifies this instance */ private long uniqueId; + /** The StatisticsFactory that created this instance */ + private final StatisticsManager statisticsManager; + /** * Suppliers of int sample values to be sampled every sample-interval */ @@ -113,12 +116,13 @@ public static Statistics createAtomicNoOS(StatisticsType type, String textId, lo * only */ public StatisticsImpl(StatisticsType type, String textId, long numericId, long uniqueId, - int osStatFlags) { + int osStatFlags, StatisticsManager statisticsManager) { this.type = (StatisticsTypeImpl) type; this.textId = textId; this.numericId = numericId; this.uniqueId = uniqueId; this.osStatFlags = osStatFlags; + this.statisticsManager = statisticsManager; closed = false; } @@ -144,6 +148,9 @@ public StatisticDescriptor nameToDescriptor(String name) { @Override public void close() { + if (this.statisticsManager != null) { + statisticsManager.destroyStatistics(this); + } this.closed = true; } diff --git a/geode-core/src/main/java/org/apache/geode/internal/statistics/StripedStatisticsImpl.java b/geode-core/src/main/java/org/apache/geode/internal/statistics/StripedStatisticsImpl.java index aa4ed9a5af67..3dc6e92dc456 100644 --- a/geode-core/src/main/java/org/apache/geode/internal/statistics/StripedStatisticsImpl.java +++ b/geode-core/src/main/java/org/apache/geode/internal/statistics/StripedStatisticsImpl.java @@ -32,8 +32,8 @@ public class StripedStatisticsImpl extends StatisticsImpl { private final DoubleAdder[] doubleAdders; public StripedStatisticsImpl(StatisticsType type, String textId, long numericId, - long uniqueId) { - super(type, textId, numericId, uniqueId, 0); + long uniqueId, StatisticsManager statisticsManager) { + super(type, textId, numericId, uniqueId, 0, statisticsManager); StatisticsTypeImpl realType = (StatisticsTypeImpl) type; @@ -47,7 +47,6 @@ public StripedStatisticsImpl(StatisticsType type, String textId, long numericId, .toArray(DoubleAdder[]::new); } - @Override public boolean isAtomic() { return true; diff --git a/geode-core/src/test/java/org/apache/geode/internal/cache/CachePerfStatsTest.java b/geode-core/src/test/java/org/apache/geode/internal/cache/CachePerfStatsTest.java index f8c18aa86200..173155d051b8 100644 --- a/geode-core/src/test/java/org/apache/geode/internal/cache/CachePerfStatsTest.java +++ b/geode-core/src/test/java/org/apache/geode/internal/cache/CachePerfStatsTest.java @@ -86,7 +86,7 @@ public void setUp() { StatisticsFactory statisticsFactory = mock(StatisticsFactory.class); Clock clock = mock(Clock.class); - statistics = spy(new StripedStatisticsImpl(statisticsType, TEXT_ID, 1, 1)); + statistics = spy(new StripedStatisticsImpl(statisticsType, TEXT_ID, 1, 1, statisticsManager)); when(clock.getTime()).thenReturn(CLOCK_TIME); when(statisticsFactory.createAtomicStatistics(eq(statisticsType), eq(TEXT_ID))) diff --git a/geode-junit/src/main/java/org/apache/geode/internal/statistics/StatUtils.java b/geode-junit/src/main/java/org/apache/geode/internal/statistics/StatUtils.java index 2fbbd2b80bf5..6be24658dd28 100644 --- a/geode-junit/src/main/java/org/apache/geode/internal/statistics/StatUtils.java +++ b/geode-junit/src/main/java/org/apache/geode/internal/statistics/StatUtils.java @@ -42,6 +42,8 @@ public class StatUtils { */ public static void compareStatArchiveFiles(final File expectedStatArchiveFile, final File actualStatArchiveFile) throws IOException { + System.out.println(actualStatArchiveFile); + System.out.println(expectedStatArchiveFile); assertThat(expectedStatArchiveFile).exists(); assertThat(actualStatArchiveFile.length()).isEqualTo(expectedStatArchiveFile.length()); From a1f566d9a04a2a7c32796ee9385d0a5fe055ab0b Mon Sep 17 00:00:00 2001 From: Jacob Barrett Date: Tue, 19 Feb 2019 13:00:25 -0800 Subject: [PATCH 4/5] GEODE-6424: Don't use even slower LocalStatisticsImpl for non-OS stats. LocalStatisticsImpl benchmarks even worse that Atomic50StatisticsImpl. It is tightly integrated with OS stats. --- .../geode/internal/concurrent/Atomics.java | 26 ------------------- .../internal/statistics/StatisticsImpl.java | 3 +-- 2 files changed, 1 insertion(+), 28 deletions(-) diff --git a/geode-core/src/main/java/org/apache/geode/internal/concurrent/Atomics.java b/geode-core/src/main/java/org/apache/geode/internal/concurrent/Atomics.java index 8ba437d21442..91f9b24a1f44 100644 --- a/geode-core/src/main/java/org/apache/geode/internal/concurrent/Atomics.java +++ b/geode-core/src/main/java/org/apache/geode/internal/concurrent/Atomics.java @@ -17,35 +17,9 @@ import java.util.concurrent.atomic.AtomicLong; -import org.apache.geode.Statistics; -import org.apache.geode.StatisticsType; -import org.apache.geode.distributed.internal.DistributionConfig; -import org.apache.geode.internal.statistics.LocalStatisticsImpl; -import org.apache.geode.internal.statistics.StatisticsManager; -import org.apache.geode.internal.statistics.StripedStatisticsImpl; - public class Atomics { private Atomics() {} - /** - * Whether per-thread stats are used. Striping is disabled for the IBM JVM due to bug 38226 - */ - private static final boolean STRIPED_STATS_DISABLED = - Boolean.getBoolean(DistributionConfig.GEMFIRE_PREFIX + "STRIPED_STATS_DISABLED") - || "IBM Corporation".equals(System.getProperty("java.vm.vendor", "unknown")); - - - public static Statistics createAtomicStatistics(StatisticsType type, String textId, long nId, - long uId, StatisticsManager mgr) { - Statistics result = null; - if (STRIPED_STATS_DISABLED) { - result = new LocalStatisticsImpl(type, textId, nId, uId, true, 0, mgr); - } else { - result = new StripedStatisticsImpl(type, textId, nId, uId, mgr); - } - return result; - } - /** * Use it only when threads are doing incremental updates. If updates are random then this method * may not be optimal. diff --git a/geode-core/src/main/java/org/apache/geode/internal/statistics/StatisticsImpl.java b/geode-core/src/main/java/org/apache/geode/internal/statistics/StatisticsImpl.java index 74c6e3b53c45..55fa50310945 100644 --- a/geode-core/src/main/java/org/apache/geode/internal/statistics/StatisticsImpl.java +++ b/geode-core/src/main/java/org/apache/geode/internal/statistics/StatisticsImpl.java @@ -27,7 +27,6 @@ import org.apache.geode.Statistics; import org.apache.geode.StatisticsType; import org.apache.geode.annotations.internal.MutableForTesting; -import org.apache.geode.internal.concurrent.Atomics; import org.apache.geode.internal.logging.LogService; import org.apache.geode.internal.util.concurrent.CopyOnWriteHashMap; @@ -102,7 +101,7 @@ public abstract class StatisticsImpl implements Statistics { */ public static Statistics createAtomicNoOS(StatisticsType type, String textId, long numericId, long uniqueId, StatisticsManager mgr) { - return Atomics.createAtomicStatistics(type, textId, numericId, uniqueId, mgr); + return new StripedStatisticsImpl(type, textId, numericId, uniqueId, mgr); } /** From 18f3cc475d63306a2c3a33c325f1258013982d85 Mon Sep 17 00:00:00 2001 From: Jacob Barrett Date: Tue, 19 Feb 2019 19:34:01 -0800 Subject: [PATCH 5/5] GEODE-6424: Revert debug changes. --- .../java/org/apache/geode/internal/statistics/StatUtils.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/geode-junit/src/main/java/org/apache/geode/internal/statistics/StatUtils.java b/geode-junit/src/main/java/org/apache/geode/internal/statistics/StatUtils.java index 6be24658dd28..2fbbd2b80bf5 100644 --- a/geode-junit/src/main/java/org/apache/geode/internal/statistics/StatUtils.java +++ b/geode-junit/src/main/java/org/apache/geode/internal/statistics/StatUtils.java @@ -42,8 +42,6 @@ public class StatUtils { */ public static void compareStatArchiveFiles(final File expectedStatArchiveFile, final File actualStatArchiveFile) throws IOException { - System.out.println(actualStatArchiveFile); - System.out.println(expectedStatArchiveFile); assertThat(expectedStatArchiveFile).exists(); assertThat(actualStatArchiveFile.length()).isEqualTo(expectedStatArchiveFile.length());