HHH-14691 Small optimisation for updating Query Cache Statistics

Author: Sanne

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

@@ -563,24 +563,21 @@ public CacheRegionStatisticsImpl getDomainDataRegionStatistics(String regionName
 	}
 
 	@Override
-	public CacheRegionStatisticsImpl getQueryRegionStatistics(String regionName) {
-		final CacheRegionStatisticsImpl existing = l2CacheStatsMap.get( regionName );
-		if ( existing != null ) {
-			return existing;
-		}
+	public CacheRegionStatisticsImpl getQueryRegionStatistics(final String regionName) {
+		return l2CacheStatsMap.getOrCompute( regionName, this::computeQueryRegionStatistics );
+	}
 
-		final QueryResultsCache regionAccess = cache
-				.getQueryResultsCacheStrictly( regionName );
+	private CacheRegionStatisticsImpl computeQueryRegionStatistics(final String regionName) {
+		final QueryResultsCache regionAccess = cache.getQueryResultsCacheStrictly( regionName );
 		if ( regionAccess == null ) {
-			return null;
+			return null; //this null value will be cached
+		}
+		else {
+			return new CacheRegionStatisticsImpl( regionAccess.getRegion() );
 		}
-
-		return l2CacheStatsMap.getOrCompute(
-				regionName,
-				s -> new CacheRegionStatisticsImpl( regionAccess.getRegion() )
-		);
 	}
 
+
 	@Override
 	public CacheRegionStatisticsImpl getCacheRegionStatistics(String regionName) {
 		if ( ! secondLevelCacheEnabled ) {

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

@@ -25,7 +25,8 @@
  */
 final class StatsNamedContainer<V> {
 
-	private final ConcurrentMap<String,V> map;
+	private final ConcurrentMap<String,Object> map;
+	private final static Object NULL_TOKEN = new Object();
 
 	/**
 	 * Creates a bounded container - based on BoundedConcurrentHashMap
@@ -63,33 +64,39 @@ public String[] keysAsArray() {
 	 * 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 );
+		final Object v1 = map.get( key );
 		if ( v1 != null ) {
-			return v1;
+			if ( v1 == NULL_TOKEN ) {
+				return null;
+			}
+			return (V) v1;
 		}
 		else {
 			final V v2 = function.apply( key );
-			//Occasionally a function might return null. We can't store a null in the CHM,
-			// so a placeholder would be required to implement that, but we prefer to just keep this
-			// situation as slightly sub-optimal so to not make the code more complex just to handle the exceptional case:
-			// null values are assumed to be rare enough for this not being worth it.
 			if ( v2 == null ) {
+				map.put( key, NULL_TOKEN );
 				return null;
 			}
 			else {
-				final V v3 = map.putIfAbsent( key, v2 );
+				final Object v3 = map.putIfAbsent( key, v2 );
 				if ( v3 == null ) {
 					return v2;
 				}
 				else {
-					return v3;
+					return (V) v3;
 				}
 			}
 		}
 	}
 
 	public V get(final String key) {
-		return map.get( key );
+		final Object o = map.get( key );
+		if ( o == NULL_TOKEN) {
+			return null;
+		}
+		else {
+			return (V) o;
+		}
 	}
 
 }

hibernate-core/src/test/java/org/hibernate/stat/internal/StatsNamedContainerNullComputedValueTest.java

@@ -6,14 +6,21 @@
  */
 package org.hibernate.stat.internal;
 
+import java.util.concurrent.atomic.AtomicInteger;
+
 import org.hibernate.testing.TestForIssue;
+import org.junit.Assert;
 import org.junit.Test;
 
+import static org.junit.Assert.assertArrayEquals;
 import static org.junit.Assert.assertNull;
 
 @TestForIssue(jiraKey = "HHH-13645")
 public class StatsNamedContainerNullComputedValueTest {
 
+	private final static AtomicInteger invocationCounterNullProducer = new AtomicInteger();
+	private final static AtomicInteger invocationCounterValueProducer = new AtomicInteger();
+
 	@Test
 	public void testNullComputedValue() {
 		final StatsNamedContainer statsNamedContainer = new StatsNamedContainer<Integer>();
@@ -27,4 +34,48 @@ public void testNullComputedValue() {
 		);
 	}
 
-}
+	@Test
+	public void abletoStoreNullValues() {
+		final StatsNamedContainer statsNamedContainer = new StatsNamedContainer<Integer>();
+		Assert.assertEquals( 0, invocationCounterNullProducer.get() );
+		assertNull(	getCacheWithNullValue( statsNamedContainer ) );
+		Assert.assertEquals( 1, invocationCounterNullProducer.get() );
+		assertNull(	getCacheWithNullValue( statsNamedContainer ) );
+		Assert.assertEquals( 1, invocationCounterNullProducer.get() );
+	}
+
+	@Test
+	public void abletoStoreActualValues() {
+		final StatsNamedContainer statsNamedContainer = new StatsNamedContainer<Integer>();
+		Assert.assertEquals( 0, invocationCounterValueProducer.get() );
+		Assert.assertEquals( 5,	getCacheWithActualValue( statsNamedContainer ) );
+		Assert.assertEquals( 1, invocationCounterValueProducer.get() );
+		Assert.assertEquals( 5,	getCacheWithActualValue( statsNamedContainer ) );
+		Assert.assertEquals( 1, invocationCounterValueProducer.get() );
+	}
+
+	private Object getCacheWithActualValue(StatsNamedContainer statsNamedContainer) {
+		return statsNamedContainer.getOrCompute(
+				"key",
+				StatsNamedContainerNullComputedValueTest::produceValue
+		);
+	}
+
+	private Object getCacheWithNullValue(StatsNamedContainer statsNamedContainer) {
+		return statsNamedContainer.getOrCompute(
+				"key",
+				StatsNamedContainerNullComputedValueTest::produceNull
+		);
+	}
+
+	private static Integer produceValue(Object o) {
+		invocationCounterValueProducer.getAndIncrement();
+		return Integer.valueOf( 5 );
+	}
+
+	private static Integer produceNull(Object v) {
+		invocationCounterNullProducer.getAndIncrement();
+		return null;
+	}
+
+}