HHH-13527 Fix contention in StatisticsImpl#getDomainDataRegionStatistics()

hibernate-core/src/main/java/org/hibernate/stat/internal/StatisticsImpl.java

@@ -6,8 +6,6 @@
  */
 package org.hibernate.stat.internal;
 
-import java.util.concurrent.ConcurrentHashMap;
-import java.util.concurrent.ConcurrentMap;
 import java.util.concurrent.atomic.AtomicLong;
 import java.util.concurrent.atomic.LongAdder;
 
@@ -16,7 +14,6 @@ import org.hibernate.cache.spi.QueryResultsRegion;
 import org.hibernate.cache.spi.Region;
 import org.hibernate.engine.spi.SessionFactoryImplementor;
 import org.hibernate.internal.CoreMessageLogger;
-import org.hibernate.internal.util.collections.ArrayHelper;
 import org.hibernate.metamodel.model.domain.NavigableRole;
 import org.hibernate.persister.entity.EntityPersister;
 import org.hibernate.service.Service;
@@ -28,6 +25,7 @@ import static org.hibernate.internal.CoreLogging.messageLogger;
  * Implementation of {@link org.hibernate.stat.Statistics} based on the {@link java.util.concurrent} package.
  *
  * @author Alex Snaps
+ * @author Sanne Grinovero
  */
 @SuppressWarnings({ "unchecked" })
 public class StatisticsImpl implements StatisticsImplementor, Service {
@@ -85,21 +83,21 @@ public class StatisticsImpl implements StatisticsImplementor, Service {
 
 	private final LongAdder optimisticFailureCount = new LongAdder();
 
-	private final ConcurrentMap<String,EntityStatisticsImpl> entityStatsMap = new ConcurrentHashMap();
+	private final StatsNamedContainer<EntityStatisticsImpl> entityStatsMap = new StatsNamedContainer();
-	private final ConcurrentMap<String,NaturalIdStatisticsImpl> naturalIdQueryStatsMap = new ConcurrentHashMap();
+	private final StatsNamedContainer<NaturalIdStatisticsImpl> naturalIdQueryStatsMap = new StatsNamedContainer();
-	private final ConcurrentMap<String,CollectionStatisticsImpl> collectionStatsMap = new ConcurrentHashMap();
+	private final StatsNamedContainer<CollectionStatisticsImpl> collectionStatsMap = new StatsNamedContainer();
 
 	/**
 	 * Keyed by query string
 	 */
-	private final ConcurrentMap<String, QueryStatisticsImpl> queryStatsMap = new ConcurrentHashMap();
+	private final StatsNamedContainer<QueryStatisticsImpl> queryStatsMap = new StatsNamedContainer();
 
 	/**
 	 * Keyed by region name
 	 */
-	private final ConcurrentMap<String,CacheRegionStatisticsImpl> l2CacheStatsMap = new ConcurrentHashMap<>();
+	private final StatsNamedContainer<CacheRegionStatisticsImpl> l2CacheStatsMap = new StatsNamedContainer<>();
 
-	private final ConcurrentMap<String,DeprecatedNaturalIdCacheStatisticsImpl> deprecatedNaturalIdStatsMap = new ConcurrentHashMap();
+	private final StatsNamedContainer<DeprecatedNaturalIdCacheStatisticsImpl> deprecatedNaturalIdStatsMap = new StatsNamedContainer();
 
 
 	@SuppressWarnings({ "UnusedDeclaration" })
@@ -197,7 +195,7 @@ public class StatisticsImpl implements StatisticsImplementor, Service {
 	@Override
 	public String[] getEntityNames() {
 		if ( sessionFactory == null ) {
-			return ArrayHelper.toStringArray( entityStatsMap.keySet() );
+			return entityStatsMap.keysAsArray();
 		}
 		else {
 			return sessionFactory.getMetamodel().getAllEntityNames();
@@ -210,7 +208,7 @@ public class StatisticsImpl implements StatisticsImplementor, Service {
 			return null;
 		}
 
-		return entityStatsMap.computeIfAbsent(
+		return entityStatsMap.getOrCompute(
 				entityName,
 				s -> new EntityStatisticsImpl( sessionFactory.getMetamodel().entityPersister( entityName ) )
 		);
@@ -310,7 +308,7 @@ public class StatisticsImpl implements StatisticsImplementor, Service {
 	@Override
 	public String[] getCollectionRoleNames() {
 		if ( sessionFactory == null ) {
-			return ArrayHelper.toStringArray( collectionStatsMap.keySet() );
+			return collectionStatsMap.keysAsArray();
 		}
 		else {
 			return sessionFactory.getMetamodel().getAllCollectionRoles();
@@ -323,7 +321,7 @@ public class StatisticsImpl implements StatisticsImplementor, Service {
 			return null;
 		}
 
-		return collectionStatsMap.computeIfAbsent(
+		return collectionStatsMap.getOrCompute(
 				role,
 				s -> new CollectionStatisticsImpl( sessionFactory.getMetamodel().collectionPersister( role ) )
 		);
@@ -415,7 +413,7 @@ public class StatisticsImpl implements StatisticsImplementor, Service {
 			return null;
 		}
 
-		return naturalIdQueryStatsMap.computeIfAbsent(
+		return naturalIdQueryStatsMap.getOrCompute(
 				rootEntityName,
 				s -> {
 					final EntityPersister entityDescriptor = sessionFactory.getMetamodel().entityPersister( rootEntityName );
@@ -429,8 +427,9 @@ public class StatisticsImpl implements StatisticsImplementor, Service {
 
 	@Override
 	public DeprecatedNaturalIdCacheStatisticsImpl getNaturalIdCacheStatistics(String regionName) {
-		return deprecatedNaturalIdStatsMap.computeIfAbsent(
+		final String key = sessionFactory.getCache().unqualifyRegionName( regionName );
-				sessionFactory.getCache().unqualifyRegionName( regionName ),
+		return deprecatedNaturalIdStatsMap.getOrCompute(
+				key,
 				unqualifiedRegionName -> new DeprecatedNaturalIdCacheStatisticsImpl(
 						unqualifiedRegionName,
 						sessionFactory.getCache().getNaturalIdAccessesInRegion( unqualifiedRegionName )
@@ -569,7 +568,7 @@ public class StatisticsImpl implements StatisticsImplementor, Service {
 			return null;
 		}
 
-		return l2CacheStatsMap.computeIfAbsent(
+		return l2CacheStatsMap.getOrCompute(
 				regionName,
 				s -> {
 					final Region region = sessionFactory.getCache().getRegion( regionName );
@@ -606,7 +605,7 @@ public class StatisticsImpl implements StatisticsImplementor, Service {
 			return null;
 		}
 
-		return l2CacheStatsMap.computeIfAbsent(
+		return l2CacheStatsMap.getOrCompute(
 				regionName,
 				s -> new CacheRegionStatisticsImpl( regionAccess.getRegion() )
 		);
@@ -622,7 +621,7 @@ public class StatisticsImpl implements StatisticsImplementor, Service {
 			return null;
 		}
 
-		return l2CacheStatsMap.computeIfAbsent(
+		return l2CacheStatsMap.getOrCompute(
 				regionName,
 				s -> {
 					Region region = sessionFactory.getCache().getRegion( regionName );
@@ -702,12 +701,12 @@ public class StatisticsImpl implements StatisticsImplementor, Service {
 
 	@Override
 	public String[] getQueries() {
-		return ArrayHelper.toStringArray( queryStatsMap.keySet() );
+		return queryStatsMap.keysAsArray();
 	}
 
 	@Override
 	public QueryStatisticsImpl getQueryStatistics(String queryString) {
-		return queryStatsMap.computeIfAbsent(
+		return queryStatsMap.getOrCompute(
 				queryString,
 				s -> new QueryStatisticsImpl( queryString )
 		);
@@ -779,7 +778,7 @@ public class StatisticsImpl implements StatisticsImplementor, Service {
 	}
 
 	private CacheRegionStatisticsImpl getQueryRegionStats(String regionName) {
-		return l2CacheStatsMap.computeIfAbsent(
+		return l2CacheStatsMap.getOrCompute(
 				regionName,
 				s -> new CacheRegionStatisticsImpl( sessionFactory.getCache().getQueryResultsCache( regionName ).getRegion() )
 		);

hibernate-core/src/main/java/org/hibernate/stat/internal/StatsNamedContainer.java

@@ -0,0 +1,71 @@
+/*
+ * Hibernate, Relational Persistence for Idiomatic Java
+ *
+ * License: GNU Lesser General Public License (LGPL), version 2.1 or later.
+ * See the lgpl.txt file in the root directory or <http://www.gnu.org/licenses/lgpl-2.1.html>.
+ */
+package org.hibernate.stat.internal;
+
+import java.util.ArrayList;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.Function;
+
+/**
+ * Decorates a ConcurrentHashMap implementation to make sure the methods are being
+ * used correctly for the purpose of Hibernate's statistics.
+ * In particular, we do like the semantics of ConcurrentHashMap#computeIfAbsent
+ * but not how it performs.
+ * See also:
+ *  - http://jsr166-concurrency.10961.n7.nabble.com/Re-ConcurrentHashMap-computeIfAbsent-td11687.html
+ *  - https://hibernate.atlassian.net/browse/HHH-13527
+ *
+ * @author Sanne Grinovero
+ */
+final class StatsNamedContainer<V> {
+
+	private final ConcurrentHashMap<String,V> map = new ConcurrentHashMap();
+
+	public void clear() {
+		map.clear();
+	}
+
+	/**
+	 * This method is inherently racy and expensive. Only use on non-hot paths, and
+	 * only to get a recent snapshot.
+	 * @return all keys in string form.
+	 */
+	public String[] keysAsArray() {
+		return map.keySet().toArray( new String[0] );
+	}
+
+	/**
+	 * Similar semantics as you'd get by invoking {@link java.util.concurrent.ConcurrentMap#computeIfAbsent(Object, Function)},
+	 * but we check for the key existence first.
+	 * This is technically a redundant check, but it has been shown to perform better when the key existing is very likely,
+	 * as in our case.
+	 * Most notably, the ConcurrentHashMap implementation might block other accesses for the sake of making
+	 * sure the function is invoked at most once: we don't need this guarantee, and prefer to reduce risk of blocking.
+	 */
+	public V getOrCompute(final String key, final Function<String, V> function) {
+		final V v1 = map.get( key );
+		if ( v1 != null ) {
+			return v1;
+		}
+		else {
+			final V v2 = function.apply( key );
+			final V v3 = map.putIfAbsent( key, v2 );
+			if ( v3 == null ) {
+				return v2;
+			}
+			else {
+				return v3;
+			}
+		}
+	}
+
+	public V get(final String key) {
+		return map.get( key );
+	}
+
+}