From e5e2931b1c7baf34e4ce46cbaeea970d4e6a233b Mon Sep 17 00:00:00 2001 From: Sanne Grinovero Date: Thu, 25 Aug 2022 16:09:00 +0100 Subject: [PATCH] HHH-15100 Limitation of metamodel imports cache causes severe performance drops in large projects --- .../domain/internal/JpaMetamodelImpl.java | 47 ++++++++++++------- .../test/hql/MetamodelBoundedCacheTest.java | 17 +++++-- 2 files changed, 42 insertions(+), 22 deletions(-) diff --git a/hibernate-core/src/main/java/org/hibernate/metamodel/model/domain/internal/JpaMetamodelImpl.java b/hibernate-core/src/main/java/org/hibernate/metamodel/model/domain/internal/JpaMetamodelImpl.java index 7a758c63e1..b7c281ecbf 100644 --- a/hibernate-core/src/main/java/org/hibernate/metamodel/model/domain/internal/JpaMetamodelImpl.java +++ b/hibernate-core/src/main/java/org/hibernate/metamodel/model/domain/internal/JpaMetamodelImpl.java @@ -70,7 +70,6 @@ import org.hibernate.type.spi.TypeConfiguration; */ public class JpaMetamodelImpl implements JpaMetamodelImplementor, Serializable { private static final EntityManagerMessageLogger log = HEMLogging.messageLogger( JpaMetamodel.class ); - private static final ImportInfo INVALID_IMPORT = new ImportInfo<>( null, null ); private static class ImportInfo { final String importedName; @@ -98,6 +97,7 @@ public class JpaMetamodelImpl implements JpaMetamodelImplementor, Serializable { private final Map, String> entityProxyInterfaceMap = new HashMap<>(); private final Map> nameToImportMap = new ConcurrentHashMap<>(); + private final Map knownInvalidnameToImportMap = new ConcurrentHashMap<>(); public JpaMetamodelImpl(TypeConfiguration typeConfiguration, JpaCompliance jpaCompliance) { @@ -281,29 +281,42 @@ public class JpaMetamodelImpl implements JpaMetamodelImplementor, Serializable { private ImportInfo resolveImport(final String name) { final ImportInfo importInfo = nameToImportMap.get( name ); + //optimal path first if ( importInfo != null ) { //noinspection unchecked - return importInfo == INVALID_IMPORT ? null : (ImportInfo) importInfo; + return (ImportInfo) importInfo; } else { - // see if the name is a fully-qualified class name - final Class loadedClass = resolveRequestedClass( name ); - if ( loadedClass == null ) { - // it is NOT a fully-qualified class name - add a marker entry so we do not keep trying later - // note that ConcurrentHashMap does not support null value so a marker entry is needed - // [HHH-14948] But only add it if the cache size isn't getting too large, as in some use cases - // the queries are dynamically generated and this cache could lead to memory leaks when left unbounded. - if ( nameToImportMap.size() < 1_000 ) { - nameToImportMap.put( name, INVALID_IMPORT ); - } + //then check the negative cache, to avoid bothering the classloader unnecessarily + if ( knownInvalidnameToImportMap.containsKey( name ) ) { return null; } else { - // it is a fully-qualified class name - add it to the cache - // so we do not keep trying later - final ImportInfo info = new ImportInfo<>( name, loadedClass ); - nameToImportMap.put( name, info ); - return info; + // see if the name is a fully-qualified class name + final Class loadedClass = resolveRequestedClass( name ); + if ( loadedClass == null ) { + // it is NOT a fully-qualified class name - add a marker entry so we do not keep trying later + // note that ConcurrentHashMap does not support null value so a marker entry is needed + // [HHH-14948] But only add it if the cache size isn't getting too large, as in some use cases + // the queries are dynamically generated and this cache could lead to memory leaks when left unbounded. + if ( knownInvalidnameToImportMap.size() < 1_000 ) { + //TODO this collection might benefit from a LRU eviction algorithm, + //we currently have no evidence for this need but this could be explored further. + //To consider that we don't have a hard dependency on a cache implementation providing LRU semantics. + //Alternatively - even better - would be to precompute all possible valid options and + //store them in nameToImportMap on bootstrap: if that can be filled with all (comprehensive) + //valid values, then there is no need for ever bothering the classloader. + knownInvalidnameToImportMap.put( name, name ); + } + return null; + } + else { + // it is a fully-qualified class name - add it to the cache + // so to not needing to load from the classloader again + final ImportInfo info = new ImportInfo<>( name, loadedClass ); + nameToImportMap.put( name, info ); + return info; + } } } } diff --git a/hibernate-core/src/test/java/org/hibernate/orm/test/hql/MetamodelBoundedCacheTest.java b/hibernate-core/src/test/java/org/hibernate/orm/test/hql/MetamodelBoundedCacheTest.java index 681aaefe51..0c09c6140e 100644 --- a/hibernate-core/src/test/java/org/hibernate/orm/test/hql/MetamodelBoundedCacheTest.java +++ b/hibernate-core/src/test/java/org/hibernate/orm/test/hql/MetamodelBoundedCacheTest.java @@ -38,15 +38,22 @@ public class MetamodelBoundedCacheTest extends BaseNonConfigCoreFunctionalTestCa jpaMetamodel.qualifyImportableName( "nonexistend" + i ); } - Field field = JpaMetamodelImpl.class.getDeclaredField( "nameToImportMap" ); - field.setAccessible( true ); + Map validImports = extractMapFromMetamodel( jpaMetamodel, "nameToImportMap" ); + Map invalidImports = extractMapFromMetamodel( jpaMetamodel, "knownInvalidnameToImportMap" ); - //noinspection unchecked - Map imports = (Map) field.get( jpaMetamodel ); + assertEquals( 2, validImports.size() ); // VERY hard-coded, but considering the possibility of a regression of a memory-related issue, // it should be worth it - assertEquals( 1000, imports.size() ); + assertEquals( 1000, invalidImports.size() ); + } + + + private Map extractMapFromMetamodel(final JpaMetamodel jpaMetamodel, final String fieldName) throws NoSuchFieldException, IllegalAccessException { + Field field = JpaMetamodelImpl.class.getDeclaredField( fieldName ); + field.setAccessible( true ); + //noinspection unchecked + return (Map) field.get( jpaMetamodel ); } @Override