From 916849a8af149b8239c423182b756dcf8ea74cec Mon Sep 17 00:00:00 2001 From: Sanne Grinovero Date: Tue, 22 Jun 2021 11:16:22 +0100 Subject: [PATCH] HHH-14691 Small optimisation for updating Query Cache Statistics --- .../stat/internal/StatisticsImpl.java | 29 +++++----- .../stat/internal/StatsNamedContainer.java | 27 ++++++---- ...tsNamedContainerNullComputedValueTest.java | 53 ++++++++++++++++++- 3 files changed, 82 insertions(+), 27 deletions(-) diff --git a/hibernate-core/src/main/java/org/hibernate/stat/internal/StatisticsImpl.java b/hibernate-core/src/main/java/org/hibernate/stat/internal/StatisticsImpl.java index 8663465fce..e02f7ecde9 100644 --- a/hibernate-core/src/main/java/org/hibernate/stat/internal/StatisticsImpl.java +++ b/hibernate-core/src/main/java/org/hibernate/stat/internal/StatisticsImpl.java @@ -563,24 +563,21 @@ public class StatisticsImpl implements StatisticsImplementor, Service, Manageabl } @Override - public CacheRegionStatisticsImpl getQueryRegionStatistics(String regionName) { - final CacheRegionStatisticsImpl existing = l2CacheStatsMap.get( regionName ); - if ( existing != null ) { - return existing; - } - - final QueryResultsCache regionAccess = cache - .getQueryResultsCacheStrictly( regionName ); - if ( regionAccess == null ) { - return null; - } - - return l2CacheStatsMap.getOrCompute( - regionName, - s -> new CacheRegionStatisticsImpl( regionAccess.getRegion() ) - ); + public CacheRegionStatisticsImpl getQueryRegionStatistics(final String regionName) { + return l2CacheStatsMap.getOrCompute( regionName, this::computeQueryRegionStatistics ); } + private CacheRegionStatisticsImpl computeQueryRegionStatistics(final String regionName) { + final QueryResultsCache regionAccess = cache.getQueryResultsCacheStrictly( regionName ); + if ( regionAccess == null ) { + return null; //this null value will be cached + } + else { + return new CacheRegionStatisticsImpl( regionAccess.getRegion() ); + } + } + + @Override public CacheRegionStatisticsImpl getCacheRegionStatistics(String regionName) { if ( ! secondLevelCacheEnabled ) { diff --git a/hibernate-core/src/main/java/org/hibernate/stat/internal/StatsNamedContainer.java b/hibernate-core/src/main/java/org/hibernate/stat/internal/StatsNamedContainer.java index dcf195a056..8e85c4aecb 100644 --- a/hibernate-core/src/main/java/org/hibernate/stat/internal/StatsNamedContainer.java +++ b/hibernate-core/src/main/java/org/hibernate/stat/internal/StatsNamedContainer.java @@ -25,7 +25,8 @@ import org.hibernate.internal.util.collections.BoundedConcurrentHashMap; */ final class StatsNamedContainer { - private final ConcurrentMap map; + private final ConcurrentMap map; + private final static Object NULL_TOKEN = new Object(); /** * Creates a bounded container - based on BoundedConcurrentHashMap @@ -63,33 +64,39 @@ final class StatsNamedContainer { * 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 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; + } } } diff --git a/hibernate-core/src/test/java/org/hibernate/stat/internal/StatsNamedContainerNullComputedValueTest.java b/hibernate-core/src/test/java/org/hibernate/stat/internal/StatsNamedContainerNullComputedValueTest.java index afd5b1088d..307ba75034 100644 --- a/hibernate-core/src/test/java/org/hibernate/stat/internal/StatsNamedContainerNullComputedValueTest.java +++ b/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(); @@ -27,4 +34,48 @@ public class StatsNamedContainerNullComputedValueTest { ); } -} \ No newline at end of file + @Test + public void abletoStoreNullValues() { + final StatsNamedContainer statsNamedContainer = new StatsNamedContainer(); + 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(); + 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; + } + +}