HHH-18976 Avoid usage of Array.newInstance #9589

yrodiere · 2025-01-08T16:01:26Z

https://hibernate.atlassian.net/browse/HHH-18976

A simpler solution than #9569, without any attempt to solve HHH-16809 (on contrary to #9576), because that makes things complicated.

Two things in here:

Sets up forbiddenapis to forbid usage of Array.newInstance (and util methods known to use it) by default. This is mostly as a heads-up to not use it "just because", since the check can be bypassed if necessary by annotating the method with @AllowReflection.
Reworks a few implementations to avoid usage of Array.newInstance. E.g.:
- use array.clone() where relevant -- i.e. when we want the same array type with the same length.
- don't use Array.newInstance just to retrieve the Class of an array type -- since we ultimately just look use that Class to look up a descriptor in a registry.
- use new Object[...] instead where possible -- e.g. when the array ends up being used internally only, and we don't leverage array type support so we only need Object[].
~~Reworks a few implementations to avoid Arrays.copyOf as well (because it relies on Array.newInstance).~~ => Not necessary, see conversation below.

This should get rid of most problematic uses of reflection when compiling to native binaries (see Jira issue).
There are still a few uses of Array.newInstance but as far as I can see they are completely justified and cannot easily be worked around: array mapping, JSON/XML serialization, instantiation of query result types, ...

yrodiere

I added comments to the main areas of changes -- the rest is mostly dumb find and replace.

@sebersole if you can (please!) confirm those commented sections look fine by you, we can probably merge and solve the upgrade issue in Quarkus?

yrodiere · 2025-01-09T11:48:19Z

rules/forbidden-apis.txt

+# Reflection-related
+@defaultMessage Use 'new Object[]' instead if possible. This forbidden method requires reflection and may not work in natively compiled applications. If you really must use this forbidden method, annotate the calling method with @AllowReflection.
+
+java.lang.reflect.Array#newInstance(java.lang.Class, int)
+java.lang.reflect.Array#newInstance(java.lang.Class, int[])
+org.hibernate.internal.util.collections.ArrayHelper#newInstance(java.lang.Class, int)
+org.hibernate.internal.util.collections.ArrayHelper#filledArray(java.lang.Object, java.lang.Class, int)
+org.hibernate.internal.util.collections.ArrayHelper#join(java.lang.Object[], java.lang.Object[])
+
+@defaultMessage Use '.clone()' or a type-specific 'ArrayHelper.copyOf' instead if possible. This forbidden method requires reflection and may not work in natively compiled applications. If you really must use this forbidden method, annotate the calling method with @AllowReflection.
+
+java.util.Arrays#copyOf(java.lang.Object[], int)
+java.util.Arrays#copyOf(java.lang.Object[], int, java.lang.Class)


I believe these constraints are reasonable -- especially since we can bypass them where necessary. Essentially this gives a heads-up to not introduce new uses of these methods by mistake, but doesn't prevent it if we need to.

yrodiere · 2025-01-09T11:56:32Z

hibernate-core/src/main/java/org/hibernate/type/descriptor/java/spi/JavaTypeRegistry.java

-	private final ConcurrentHashMap<Type, JavaType<?>> descriptorsByType = new ConcurrentHashMap<>();
+	private final ConcurrentHashMap<String, JavaType<?>> descriptorsByTypeName = new ConcurrentHashMap<>();


This (and other changes in the same file) avoids the need to use reflection to create an array Type, just for the purpose of retrieving the corresponding JavaType.

Regardless of the original intent, I believe using type names (strings) as keys would be more consistent with BasicTypeRegistry? (see for example org.hibernate.type.BasicTypeRegistry#getRegisteredType(java.lang.reflect.Type))

yrodiere · 2025-01-09T11:58:15Z

hibernate-core/src/main/java/org/hibernate/internal/MultiIdentifierLoadAccessImpl.java

-					ids.toArray( LoaderHelper.createTypedArray( ids.get( 0 ).getClass(), ids.size() ) ),
+					ids.toArray( new Object[0] ),


Note this is still done, but deeper within the multiLoad call, and only when necessary (see MultiIdEntityLoaderArrayParam).

yrodiere · 2025-01-09T12:02:42Z

hibernate-core/src/main/java/org/hibernate/loader/ast/internal/AbstractMultiIdEntityLoader.java


 	public AbstractMultiIdEntityLoader(EntityMappingType entityDescriptor, SessionFactoryImplementor sessionFactory) {
 		this.entityDescriptor = entityDescriptor;
 		this.sessionFactory = sessionFactory;
 		identifierMapping = getLoadable().getIdentifierMapping();
-		idArray = (Object[]) Array.newInstance( identifierMapping.getJavaType().getJavaTypeClass(), 0 );


This was the main problem affecting Quarkus, FWIW.
This being done in the abstract class meant it was done for MultiIdEntityLoaderArrayParam -- where it's needed, and generally innocuous since it handles things like Integer[] -- but also MultiIdEntityLoaderStandardParam -- where it's not needed, and possibly problematic as it could be handling things like MyIdClass[], requiring MyIdClass[] (the array type) to be registered for reflection in native images.
It's not trivial to detect all ID types in Quarkus (think of orm.xml using property access, or IDs defined in mappedsuperclasses with generics), so this need for registering MyIdClass[] for reflection was a problem.

hibernate-core/src/main/java/org/hibernate/type/descriptor/java/spi/JavaTypeRegistry.java

franz1981 · 2025-01-09T15:11:43Z

@yrodiere I would ask the JDK folks, because Arrays.copyOf is not actually executing that Java code (in Hotspot), since it's an intrinsics candidate (which means Hotspot will replace the Java code with a completely different optimized ad-hoc code): see https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/util/Arrays.java#L3505C7-L3505C24

yrodiere · 2025-01-09T15:23:43Z

@yrodiere I would ask the JDK folks, because Arrays.copyOf is not actually executing that Java code (in Hotspot), since it's an intrinsics candidate (which means Hotspot will replace the Java code with a completely different optimized ad-hoc code): see https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/util/Arrays.java#L3505C7-L3505C24

@franz1981 Okay, I must admit the last part about copyOf is from memory -- I remember problems with it with native compilation as well, but I may be wrong.
That being said... the problem is precisely with native image compilation with GraalVM, so does Hotspot even matter?

franz1981 · 2025-01-09T16:00:02Z

the problem is precisely with native image compilation with GraalVM, so does Hotspot even matter?

I can tell you when it could matter: the reason why is an intrinsics is because the zeroing of the array "could" be avoided between the creation of the array and the actual copy - while we loose this advantage by performing ourselves the array creation + copy (in theory).
In short; I would make sure with a benchmark that it does things without impacting JVM mode since is what the most of users are using

yrodiere · 2025-01-09T16:06:36Z

the problem is precisely with native image compilation with GraalVM, so does Hotspot even matter?

I can tell you when it could matter: the reason why is an intrinsics is because the zeroing of the array "could" be avoided between the creation of the array and the actual copy - while we loose this advantage by performing ourselves the array creation + copy (in theory). In short; I would make sure with a benchmark that it does things without impacting JVM mode since is what the most of users are using

I can have the ArrayHelper.copyOf implementations just delegate to Arrays.copyOf? Then they would only serve as a way to clarify "we don't do reflection here, we only do String[] or Object[]". And no need for a benchmark.

EDIT: well we would potentially do reflection with GraalVM, but just for String[]/Object[], which is reasonable

yrodiere · 2025-01-09T16:54:03Z

I'll also double check in Quarkus that Arrays.copyOf actually requires reflection like Array.newInstance does...

…Array#newInstance

…lans It's more consistent, and happens to get rid of ArrayMutabilityPlan, which involved an unnecessary use of Array.newInstance. I've also seen claims that clone() performs better than Array.newInstance() due to not having to zero-out the allocated memory, but I doubt that's relevant here.

No functional impact, it's just less redundant.

…e type descriptors

…Standard MultiEntityLoaderStandard is used for arbitrary ID types, including IdClass, making it very problematic to instantiate T[] where T is the ID type: in native images, it requires registering T[] for reflection for every T that can possibly be used as an ID type. Fortunately, MultiEntityLoaderStandard does not, in fact, need concrete-type arrays: Object[] works perfectly well with this implementation, and only the other implementation, MultiIdEntityLoaderArrayParam, actually needs concrete-type arrays. We're truly in a lucky streak, because MultiIdEntityLoaderArrayParam is only used for well-known, basic types such as Integer, which can easily be registered for reflection in native images, and likely will be for other reasons anyway. Some of the dynamic instantiations were originally introduced to fix the following issue: * HHH-17201 -- tested in MultiIdEntityLoadTests The corresponding tests still pass after removing these dynamic array instantiations.

yrodiere · 2025-01-10T13:15:31Z

I'll also double check in Quarkus that Arrays.copyOf actually requires reflection like Array.newInstance does...

It does not, for some reason.

I removed the commits about Arrays.copyOf. Thanks for the heads-up @franz1981!

I also checked (locally) that this changeset solves the specific reflection-related problem we had in my PR to upgrade Quarkus to ORM 7.0 (quarkusio/quarkus#41310).

yrodiere · 2025-01-14T08:07:42Z

Steve is likely busy, but others have reviewed and I addressed objections.

Let's merge :)

yrodiere force-pushed the HHH-18976 branch 3 times, most recently from b581178 to 4340eab Compare January 9, 2025 08:42

yrodiere commented Jan 9, 2025

View reviewed changes

This was referenced Jan 9, 2025

Register arrays of Hibernate entity ID types for reflection quarkusio/quarkus#45236

Closed

HHH-16809 + HHH-18976 Avoid usage of Array.newInstance + Add JavaType#newArray #9576

Closed

beikov reviewed Jan 9, 2025

View reviewed changes

hibernate-core/src/main/java/org/hibernate/type/descriptor/java/spi/JavaTypeRegistry.java Outdated Show resolved Hide resolved

yrodiere force-pushed the HHH-18976 branch from 4340eab to f9c2029 Compare January 9, 2025 13:18

yrodiere added 6 commits January 10, 2025 14:12

HHH-18976 Forbid any future new uses of Array.newInstance

3741a62

HHH-18976 Deprecate ArrayHelper#newInstance in favor of the standard …

c50c8be

…Array#newInstance

HHH-18976 Simplify MultiKeyLoadHelper#resolveArrayJdbcMapping

06cbf3b

No functional impact, it's just less redundant.

HHH-18976 Get rid of dynamic array instantiations used only to resolv…

d7551cb

…e type descriptors

yrodiere force-pushed the HHH-18976 branch from f9c2029 to d9d2fbc Compare January 10, 2025 13:13

yrodiere merged commit 5ffff1b into hibernate:main Jan 14, 2025
25 checks passed

yrodiere mentioned this pull request Jan 14, 2025

[WIP] Upgrade to Hibernate ORM 7 quarkusio/quarkus#41310

Draft

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HHH-18976 Avoid usage of Array.newInstance #9589

HHH-18976 Avoid usage of Array.newInstance #9589

yrodiere commented Jan 8, 2025 •

edited

Loading

yrodiere left a comment

yrodiere Jan 9, 2025

yrodiere Jan 9, 2025

yrodiere Jan 9, 2025

yrodiere Jan 9, 2025

franz1981 commented Jan 9, 2025

yrodiere commented Jan 9, 2025 •

edited

Loading

franz1981 commented Jan 9, 2025

yrodiere commented Jan 9, 2025 •

edited

Loading

yrodiere commented Jan 9, 2025 •

edited

Loading

yrodiere commented Jan 10, 2025

yrodiere commented Jan 14, 2025

		private final ConcurrentHashMap<Type, JavaType<?>> descriptorsByType = new ConcurrentHashMap<>();
		private final ConcurrentHashMap<String, JavaType<?>> descriptorsByTypeName = new ConcurrentHashMap<>();

		ids.toArray( LoaderHelper.createTypedArray( ids.get( 0 ).getClass(), ids.size() ) ),
		ids.toArray( new Object[0] ),

HHH-18976 Avoid usage of Array.newInstance #9589

HHH-18976 Avoid usage of Array.newInstance #9589

Conversation

yrodiere commented Jan 8, 2025 • edited Loading

yrodiere left a comment

Choose a reason for hiding this comment

yrodiere Jan 9, 2025

Choose a reason for hiding this comment

yrodiere Jan 9, 2025

Choose a reason for hiding this comment

yrodiere Jan 9, 2025

Choose a reason for hiding this comment

yrodiere Jan 9, 2025

Choose a reason for hiding this comment

franz1981 commented Jan 9, 2025

yrodiere commented Jan 9, 2025 • edited Loading

franz1981 commented Jan 9, 2025

yrodiere commented Jan 9, 2025 • edited Loading

yrodiere commented Jan 9, 2025 • edited Loading

yrodiere commented Jan 10, 2025

yrodiere commented Jan 14, 2025

yrodiere commented Jan 8, 2025 •

edited

Loading

yrodiere commented Jan 9, 2025 •

edited

Loading

yrodiere commented Jan 9, 2025 •

edited

Loading

yrodiere commented Jan 9, 2025 •

edited

Loading