From c092acb037b11a85a1a2e4725bd0340952dba683 Mon Sep 17 00:00:00 2001 From: fabrizio giovannetti Date: Sat, 25 Jan 2025 14:22:59 +0100 Subject: [PATCH] optimization draft --- gradle.properties | 2 +- .../test/context/cache/ContextCache.java | 9 ++- ...efaultCacheAwareContextLoaderDelegate.java | 77 +++++++++---------- .../context/cache/DefaultContextCache.java | 75 +++++++++++++++--- 4 files changed, 108 insertions(+), 55 deletions(-) diff --git a/gradle.properties b/gradle.properties index 37459f39dcab..1e6df0d3afc7 100644 --- a/gradle.properties +++ b/gradle.properties @@ -1,4 +1,4 @@ -version=7.0.0-SNAPSHOT +version=7.0.0b-SNAPSHOT org.gradle.caching=true org.gradle.jvmargs=-Xmx2048m diff --git a/spring-test/src/main/java/org/springframework/test/context/cache/ContextCache.java b/spring-test/src/main/java/org/springframework/test/context/cache/ContextCache.java index f4f4a6fe1983..56816206a58b 100644 --- a/spring-test/src/main/java/org/springframework/test/context/cache/ContextCache.java +++ b/spring-test/src/main/java/org/springframework/test/context/cache/ContextCache.java @@ -17,11 +17,13 @@ package org.springframework.test.context.cache; import org.jspecify.annotations.Nullable; - import org.springframework.context.ApplicationContext; import org.springframework.test.annotation.DirtiesContext.HierarchyMode; import org.springframework.test.context.MergedContextConfiguration; +import java.util.concurrent.Future; +import java.util.function.Function; + /** * {@code ContextCache} defines the SPI for caching Spring * {@link ApplicationContext ApplicationContexts} within the @@ -96,7 +98,10 @@ public interface ContextCache { * if not found in the cache * @see #remove */ - @Nullable ApplicationContext get(MergedContextConfiguration key); + @Nullable + ApplicationContext get(MergedContextConfiguration key); + + Future computeIfAbsent(MergedContextConfiguration key, Function mappingFunction); /** * Explicitly add an {@code ApplicationContext} instance to the cache diff --git a/spring-test/src/main/java/org/springframework/test/context/cache/DefaultCacheAwareContextLoaderDelegate.java b/spring-test/src/main/java/org/springframework/test/context/cache/DefaultCacheAwareContextLoaderDelegate.java index be0bd587a72b..54d5db40394e 100644 --- a/spring-test/src/main/java/org/springframework/test/context/cache/DefaultCacheAwareContextLoaderDelegate.java +++ b/spring-test/src/main/java/org/springframework/test/context/cache/DefaultCacheAwareContextLoaderDelegate.java @@ -17,6 +17,7 @@ package org.springframework.test.context.cache; import java.util.List; +import java.util.concurrent.ExecutionException; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; @@ -125,43 +126,58 @@ private DefaultCacheAwareContextLoaderDelegate(ContextCache contextCache, int fa @Override public boolean isContextLoaded(MergedContextConfiguration mergedConfig) { mergedConfig = replaceIfNecessary(mergedConfig); - synchronized (this.contextCache) { return this.contextCache.contains(mergedConfig); } - } @Override public ApplicationContext loadContext(MergedContextConfiguration mergedConfig) { mergedConfig = replaceIfNecessary(mergedConfig); - synchronized (this.contextCache) { - ApplicationContext context = this.contextCache.get(mergedConfig); + try { - if (context == null) { - int failureCount = this.contextCache.getFailureCount(mergedConfig); + var contextLoader = this.contextCache.computeIfAbsent(mergedConfig, this::loadContextForReal); + + var context = contextLoader.get(); + + if (context != null && logger.isTraceEnabled()) { + logger.trace("Retrieved ApplicationContext [%s] from cache with key %s".formatted( + System.identityHashCode(context), mergedConfig)); + } + + return context; + } catch (InterruptedException e) { + throw new RuntimeException(e); //FIXME: Better message + } catch (ExecutionException e) { + throw new RuntimeException(e); //FIXME: Better message + } finally { + this.contextCache.logStatistics(); + } + } + + private ApplicationContext loadContextForReal(MergedContextConfiguration k) { + int failureCount = this.contextCache.getFailureCount(k); if (failureCount >= this.failureThreshold) { throw new IllegalStateException(""" ApplicationContext failure threshold (%d) exceeded: \ skipping repeated attempt to load context for %s""" - .formatted(this.failureThreshold, mergedConfig)); + .formatted(this.failureThreshold, k)); } try { - if (mergedConfig instanceof AotMergedContextConfiguration aotMergedConfig) { - context = loadContextInAotMode(aotMergedConfig); - } - else { - context = loadContextInternal(mergedConfig); + ApplicationContext contextToReturn; + if (k instanceof AotMergedContextConfiguration aotMergedConfig) { + contextToReturn = loadContextInAotMode(aotMergedConfig); + } else { + contextToReturn = loadContextInternal(k); } if (logger.isTraceEnabled()) { logger.trace("Storing ApplicationContext [%s] in cache under key %s".formatted( - System.identityHashCode(context), mergedConfig)); + System.identityHashCode(contextToReturn), k)); } - this.contextCache.put(mergedConfig, context); - } - catch (Exception ex) { + return contextToReturn; + } catch (Exception ex) { if (logger.isTraceEnabled()) { - logger.trace("Incrementing ApplicationContext failure count for " + mergedConfig); + logger.trace("Incrementing ApplicationContext failure count for " + k); } - this.contextCache.incrementFailureCount(mergedConfig); + this.contextCache.incrementFailureCount(k); Throwable cause = ex; if (ex instanceof ContextLoadException cle) { cause = cle.getCause(); @@ -178,38 +194,15 @@ ApplicationContext failure threshold (%d) exceeded: \ } } throw new IllegalStateException( - "Failed to load ApplicationContext for " + mergedConfig, cause); - } - } - else { - if (logger.isTraceEnabled()) { - logger.trace("Retrieved ApplicationContext [%s] from cache with key %s".formatted( - System.identityHashCode(context), mergedConfig)); - } - } - } - finally { - this.contextCache.logStatistics(); - } - - return context; + "Failed to load ApplicationContext for " + k, cause); } } @Override public void closeContext(MergedContextConfiguration mergedConfig, @Nullable HierarchyMode hierarchyMode) { mergedConfig = replaceIfNecessary(mergedConfig); - synchronized (this.contextCache) { this.contextCache.remove(mergedConfig, hierarchyMode); } - } - - /** - * Get the {@link ContextCache} used by this context loader delegate. - */ - protected ContextCache getContextCache() { - return this.contextCache; - } /** * Load the {@code ApplicationContext} for the supplied merged context configuration. diff --git a/spring-test/src/main/java/org/springframework/test/context/cache/DefaultContextCache.java b/spring-test/src/main/java/org/springframework/test/context/cache/DefaultContextCache.java index 6821dd8ff7d4..6e16718efa86 100644 --- a/spring-test/src/main/java/org/springframework/test/context/cache/DefaultContextCache.java +++ b/spring-test/src/main/java/org/springframework/test/context/cache/DefaultContextCache.java @@ -23,13 +23,14 @@ import java.util.List; import java.util.Map; import java.util.Set; -import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.*; import java.util.concurrent.atomic.AtomicInteger; +import java.util.function.Function; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; -import org.jspecify.annotations.Nullable; +import org.jspecify.annotations.Nullable; import org.springframework.context.ApplicationContext; import org.springframework.context.ConfigurableApplicationContext; import org.springframework.core.style.ToStringCreator; @@ -57,11 +58,12 @@ public class DefaultContextCache implements ContextCache { private static final Log statsLogger = LogFactory.getLog(CONTEXT_CACHE_LOGGING_CATEGORY); + ExecutorService executorService = Executors.newFixedThreadPool(Runtime.getRuntime().availableProcessors()); //TODO: Make this parametric /** * Map of context keys to Spring {@code ApplicationContext} instances. */ - private final Map contextMap = + private final Map> contextMap = Collections.synchronizedMap(new LruCache(32, 0.75f)); /** @@ -120,25 +122,69 @@ public boolean contains(MergedContextConfiguration key) { return this.contextMap.containsKey(key); } - @Override + @Override//TODO: This is not used anymore in spring but make sense to keep it for retro compatibility, right? public @Nullable ApplicationContext get(MergedContextConfiguration key) { Assert.notNull(key, "Key must not be null"); - ApplicationContext context = this.contextMap.get(key); + + try { + Future context = this.contextMap.get(key); + if (context == null) { this.missCount.incrementAndGet(); + + return null; } else { + this.hitCount.incrementAndGet(); + + return context.get(); + } + } catch (InterruptedException e) { + throw new RuntimeException(e);//FIXME: fix the message + } catch (ExecutionException e) { + throw new RuntimeException(e);//FIXME: fix the message + } + } + + + @Override + public Future computeIfAbsent(MergedContextConfiguration key, Function mappingFunction) { + Assert.notNull(key, "Key must not be null"); + + if(contextMap.containsKey(key)) { this.hitCount.incrementAndGet(); } - return context; + + return contextMap.computeIfAbsent(key, (k) -> + { + this.missCount.incrementAndGet(); + return CompletableFuture.supplyAsync(() -> mappingFunction.apply(k), executorService) + .thenApply( + (contextLoaded) -> { + MergedContextConfiguration child = key; + MergedContextConfiguration parent = child.getParent(); + while (parent != null) { + Set list = this.hierarchyMap.computeIfAbsent(parent, k2 -> new HashSet<>()); + list.add(child); + child = parent; + parent = child.getParent(); + } + + return contextLoaded; + } + ); + + } + ); } + //TODO: This is not used anymore in spring but make sense to keep it for retro compatibility, right? @Override public void put(MergedContextConfiguration key, ApplicationContext context) { Assert.notNull(key, "Key must not be null"); Assert.notNull(context, "ApplicationContext must not be null"); - this.contextMap.put(key, context); + this.contextMap.put(key, CompletableFuture.completedFuture(context)); MergedContextConfiguration child = key; MergedContextConfiguration parent = child.getParent(); while (parent != null) { @@ -198,10 +244,19 @@ private void remove(List removedContexts, MergedCont // Physically remove and close leaf nodes first (i.e., on the way back up the // stack as opposed to prior to the recursive call). - ApplicationContext context = this.contextMap.remove(key); + Future contextLoader = this.contextMap.remove(key); + + try { + ApplicationContext context = contextLoader.get(); if (context instanceof ConfigurableApplicationContext cac) { cac.close(); } + } catch (InterruptedException e) { + throw new RuntimeException(e); //FIXME: fix the message + } catch (ExecutionException e) { + throw new RuntimeException(e); //FIXME: fix the message + } + removedContexts.add(key); } @@ -303,7 +358,7 @@ public String toString() { * @since 4.3 */ @SuppressWarnings("serial") - private class LruCache extends LinkedHashMap { + private class LruCache extends LinkedHashMap> { /** * Create a new {@code LruCache} with the supplied initial capacity @@ -316,7 +371,7 @@ private class LruCache extends LinkedHashMap eldest) { + protected boolean removeEldestEntry(Map.Entry> eldest) { if (this.size() > DefaultContextCache.this.getMaxSize()) { // Do NOT delete "DefaultContextCache.this."; otherwise, we accidentally // invoke java.util.Map.remove(Object, Object).