From 5f9659408b322659bc0007f450db011e3fd2baea Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Thu, 6 Mar 2025 12:23:09 +0100 Subject: [PATCH] Revise RepositoryInformation and RepositoryComposition caching. We now use a refined strategy to cache RepositoryInformation and RepositoryComposition. Previously, RepositoryComposition wasn't cached at all and store modules that e.g. contributed a Querydsl (or a different) fragment based on the interface declaration returned a new RepositoryComposition (and thus a different hashCode) each time RepositoryInformation was obtained leading to memory leaks caused by HashMap caching. We now use Fragment's hashCode for the cache key resulting in RepositoryComposition being created only once for a given repository interface and input-fragments arrangement. Closes #3252 --- .../core/support/RepositoryComposition.java | 5 + .../support/RepositoryFactorySupport.java | 96 ++++++++----------- .../core/support/DummyRepositoryFactory.java | 3 +- .../RepositoryFactorySupportUnitTests.java | 25 +++++ 4 files changed, 73 insertions(+), 56 deletions(-) diff --git a/src/main/java/org/springframework/data/repository/core/support/RepositoryComposition.java b/src/main/java/org/springframework/data/repository/core/support/RepositoryComposition.java index 41c46d97e..a01da71cd 100644 --- a/src/main/java/org/springframework/data/repository/core/support/RepositoryComposition.java +++ b/src/main/java/org/springframework/data/repository/core/support/RepositoryComposition.java @@ -541,6 +541,10 @@ public class RepositoryComposition { return null; } + List> getFragments() { + return fragments; + } + /** * Returns the number of {@link RepositoryFragment fragments} available. * @@ -585,5 +589,6 @@ public class RepositoryComposition { result = (31 * result) + ObjectUtils.nullSafeHashCode(fragments); return result; } + } } diff --git a/src/main/java/org/springframework/data/repository/core/support/RepositoryFactorySupport.java b/src/main/java/org/springframework/data/repository/core/support/RepositoryFactorySupport.java index b5f51c395..39c48f164 100644 --- a/src/main/java/org/springframework/data/repository/core/support/RepositoryFactorySupport.java +++ b/src/main/java/org/springframework/data/repository/core/support/RepositoryFactorySupport.java @@ -110,7 +110,7 @@ public abstract class RepositoryFactorySupport CONVERSION_SERVICE.removeConvertible(Object.class, Object.class); } - private final Map repositoryInformationCache; + private final Map repositoryInformationCache; private final List postProcessors; private @Nullable Class repositoryBaseClass; @@ -130,7 +130,7 @@ public abstract class RepositoryFactorySupport @SuppressWarnings("null") public RepositoryFactorySupport() { - this.repositoryInformationCache = new HashMap<>(16); + this.repositoryInformationCache = new HashMap<>(8); this.postProcessors = new ArrayList<>(); this.namedQueries = PropertiesBasedNamedQueries.EMPTY; @@ -294,16 +294,6 @@ public abstract class RepositoryFactorySupport return RepositoryFragments.empty(); } - /** - * Creates {@link RepositoryComposition} based on {@link RepositoryMetadata} for repository-specific method handling. - * - * @param metadata the repository metadata to use. - * @return repository composition. - */ - private RepositoryComposition getRepositoryComposition(RepositoryMetadata metadata) { - return RepositoryComposition.fromMetadata(metadata); - } - /** * Returns a repository instance for the given interface. * @@ -362,8 +352,9 @@ public abstract class RepositoryFactorySupport repositoryInterface); repositoryCompositionStep.tag("fragment.count", String.valueOf(fragments.size())); - RepositoryComposition composition = getRepositoryComposition(metadata, fragments); - RepositoryInformation information = getRepositoryInformation(metadata, composition); + RepositorySub stub = getRepositoryStub(metadata, fragments); + RepositoryComposition composition = stub.composition(); + RepositoryInformation information = stub.information(); repositoryCompositionStep.tag("fragments", () -> { @@ -493,47 +484,35 @@ public abstract class RepositoryFactorySupport * @return will never be {@literal null}. */ protected RepositoryInformation getRepositoryInformation(RepositoryMetadata metadata, RepositoryFragments fragments) { - return getRepositoryInformation(metadata, getRepositoryComposition(metadata, fragments)); + return getRepositoryStub(metadata, fragments).information(); } /** - * Returns the {@link RepositoryComposition} for the given {@link RepositoryMetadata} and extra - * {@link RepositoryFragments}. - * - * @param metadata must not be {@literal null}. - * @param fragments must not be {@literal null}. - * @return will never be {@literal null}. - */ - private RepositoryComposition getRepositoryComposition(RepositoryMetadata metadata, RepositoryFragments fragments) { - - Assert.notNull(metadata, "RepositoryMetadata must not be null"); - Assert.notNull(fragments, "RepositoryFragments must not be null"); - - RepositoryComposition composition = getRepositoryComposition(metadata); - RepositoryFragments repositoryAspects = getRepositoryFragments(metadata); - - return composition.append(fragments).append(repositoryAspects); - } - - /** - * Returns the {@link RepositoryInformation} for the given repository interface. + * Returns the cached {@link RepositorySub} for the given repository and composition. {@link RepositoryMetadata} is a + * strong cache key while {@link RepositoryFragments} contributes a light-weight caching component by using only the + * fragments hash code. In a typical Spring scenario, that shouldn't impose issues as one repository factory produces + * only a single repository instance for one repository interface. Things might be different when using various + * fragments for the same repository interface. * * @param metadata - * @param composition + * @param fragments * @return */ - private RepositoryInformation getRepositoryInformation(RepositoryMetadata metadata, - RepositoryComposition composition) { + private RepositorySub getRepositoryStub(RepositoryMetadata metadata, RepositoryFragments fragments) { - RepositoryInformationCacheKey cacheKey = new RepositoryInformationCacheKey(metadata, composition); + RepositoryInformationCacheKey cacheKey = new RepositoryInformationCacheKey(metadata, fragments); synchronized (repositoryInformationCache) { return repositoryInformationCache.computeIfAbsent(cacheKey, key -> { + RepositoryComposition composition = RepositoryComposition.fromMetadata(metadata); + RepositoryFragments repositoryAspects = getRepositoryFragments(metadata); + composition = composition.append(fragments).append(repositoryAspects); + Class baseClass = repositoryBaseClass != null ? repositoryBaseClass : getRepositoryBaseClass(metadata); - return new DefaultRepositoryInformation(metadata, baseClass, composition); + return new RepositorySub(new DefaultRepositoryInformation(metadata, baseClass, composition), composition); }); } } @@ -816,6 +795,18 @@ public abstract class RepositoryFactorySupport } } + /** + * Repository stub holding {@link RepositoryInformation} and {@link RepositoryComposition}. + * + * @param information + * @param composition + * @author Mark Paluch + * @since 3.4.4 + */ + record RepositorySub(RepositoryInformation information, RepositoryComposition composition) { + + } + /** * Simple value object to build up keys to cache {@link RepositoryInformation} instances. * @@ -825,31 +816,26 @@ public abstract class RepositoryFactorySupport private static final class RepositoryInformationCacheKey { private final String repositoryInterfaceName; - private final long compositionHash; + private final long fragmentsHash; /** - * Creates a new {@link RepositoryInformationCacheKey} for the given {@link RepositoryMetadata} and composition. + * Creates a new {@link RepositoryInformationCacheKey} for the given {@link RepositoryMetadata} and fragments. * * @param metadata must not be {@literal null}. - * @param composition must not be {@literal null}. + * @param fragments must not be {@literal null}. */ - public RepositoryInformationCacheKey(RepositoryMetadata metadata, RepositoryComposition composition) { + public RepositoryInformationCacheKey(RepositoryMetadata metadata, RepositoryFragments fragments) { this.repositoryInterfaceName = metadata.getRepositoryInterface().getName(); - this.compositionHash = composition.hashCode(); - } - - public RepositoryInformationCacheKey(String repositoryInterfaceName, long compositionHash) { - this.repositoryInterfaceName = repositoryInterfaceName; - this.compositionHash = compositionHash; + this.fragmentsHash = fragments.getFragments().hashCode(); } public String getRepositoryInterfaceName() { return this.repositoryInterfaceName; } - public long getCompositionHash() { - return this.compositionHash; + public long getFragmentsHash() { + return this.fragmentsHash; } @Override @@ -860,7 +846,7 @@ public abstract class RepositoryFactorySupport if (!(o instanceof RepositoryInformationCacheKey that)) { return false; } - if (compositionHash != that.compositionHash) { + if (fragmentsHash != that.fragmentsHash) { return false; } return ObjectUtils.nullSafeEquals(repositoryInterfaceName, that.repositoryInterfaceName); @@ -869,14 +855,14 @@ public abstract class RepositoryFactorySupport @Override public int hashCode() { int result = ObjectUtils.nullSafeHashCode(repositoryInterfaceName); - result = 31 * result + (int) (compositionHash ^ (compositionHash >>> 32)); + result = 31 * result + Long.hashCode(fragmentsHash); return result; } @Override public String toString() { return "RepositoryFactorySupport.RepositoryInformationCacheKey(repositoryInterfaceName=" - + this.getRepositoryInterfaceName() + ", compositionHash=" + this.getCompositionHash() + ")"; + + this.getRepositoryInterfaceName() + ", fragmentsHash=" + this.getFragmentsHash() + ")"; } } diff --git a/src/test/java/org/springframework/data/repository/core/support/DummyRepositoryFactory.java b/src/test/java/org/springframework/data/repository/core/support/DummyRepositoryFactory.java index 006508c6d..eba849e44 100644 --- a/src/test/java/org/springframework/data/repository/core/support/DummyRepositoryFactory.java +++ b/src/test/java/org/springframework/data/repository/core/support/DummyRepositoryFactory.java @@ -23,6 +23,7 @@ import java.util.function.Supplier; import org.mockito.ArgumentMatchers; import org.mockito.Mockito; + import org.springframework.beans.factory.BeanFactory; import org.springframework.core.metrics.ApplicationStartup; import org.springframework.core.metrics.StartupStep; @@ -103,7 +104,7 @@ public class DummyRepositoryFactory extends RepositoryFactorySupport { var fragments = super.getRepositoryFragments(metadata); return QuerydslPredicateExecutor.class.isAssignableFrom(metadata.getRepositoryInterface()) // - ? fragments.append(RepositoryFragments.just(querydsl)) // + ? fragments.append(RepositoryFragments.just(querydsl, new Object())) // : fragments; } diff --git a/src/test/java/org/springframework/data/repository/core/support/RepositoryFactorySupportUnitTests.java b/src/test/java/org/springframework/data/repository/core/support/RepositoryFactorySupportUnitTests.java index b02d654f0..e10c2b914 100755 --- a/src/test/java/org/springframework/data/repository/core/support/RepositoryFactorySupportUnitTests.java +++ b/src/test/java/org/springframework/data/repository/core/support/RepositoryFactorySupportUnitTests.java @@ -24,6 +24,7 @@ import java.io.Serializable; import java.lang.reflect.Method; import java.util.Collections; import java.util.List; +import java.util.Map; import java.util.Optional; import java.util.Set; import java.util.concurrent.CompletableFuture; @@ -293,6 +294,26 @@ class RepositoryFactorySupportUnitTests { assertThat(metadata.methodInvocation().getMethod().getName()).isEqualTo("findMetadataByLastname"); } + @Test + void cachesRepositoryInformation() { + + var repository1 = factory.getRepository(ObjectAndQuerydslRepository.class, backingRepo); + var repository2 = factory.getRepository(ObjectAndQuerydslRepository.class, backingRepo); + repository1.findByFoo(); + repository2.deleteAll(); + + for (int i = 0; i < 10; i++) { + RepositoryFragments fragments = RepositoryFragments.just(backingRepo); + RepositoryMetadata metadata = factory.getRepositoryMetadata(ObjectAndQuerydslRepository.class); + factory.getRepositoryInformation(metadata, fragments); + } + + Map cache = (Map) ReflectionTestUtils.getField(factory, + "repositoryInformationCache"); + + assertThat(cache).hasSize(1); + } + @Test // DATACMNS-509, DATACMNS-1764 void convertsWithSameElementType() { @@ -544,6 +565,10 @@ class RepositoryFactorySupportUnitTests { interface SimpleRepository extends Repository {} + interface ObjectAndQuerydslRepository extends ObjectRepository, QuerydslPredicateExecutor { + + } + interface ObjectRepository extends Repository, ObjectRepositoryCustom { @Nullable