RFR: 8358326: Use oopFactory array allocation

[Coleen Phillimore]

On Mon, 9 Jun 2025 07:36:32 GMT, Stefan Karlsson <stefank at openjdk.org> wrote:

>> This patch removes cases of direct calls to {type,obj}ArrayKlass->allocate() and calls oopFactory::new_*array instead.  It also renames {type,obj}ArrayKlass->allocate functions to allocate_klass and allocate_instance so it's more clear which allocation it's doing and to match InstanceKlass allocate functions, and makes these functions private with friends for Deoptimization and oopFactory.  For JEP 401, arrays are being extended to support new formats and attributes and this reduces the call sites.
>> Tested with tier1-7.
>
> src/hotspot/share/oops/objArrayKlass.hpp line 39:
> 
>> 37:   friend class JVMCIVMStructs;
>> 38:   friend class oopFactory;
>> 39:   friend class Deoptimization;
> 
> If you want you could consider sorting the friend declarations (here and in the other place where you added it)

Do we sort friends? The sorting looks funny since VMStructs is usually at the beginning.

 class ObjArrayKlass : public ArrayKlass {
-  friend class VMStructs;
+  friend class Deoptimization;
   friend class JVMCIVMStructs;
   friend class oopFactory;
-  friend class Deoptimization;
+  friend class VMStructs;

> src/hotspot/share/oops/objArrayKlass.hpp line 81:
> 
>> 79:                                                 int n, Klass* element_klass, TRAPS);
>> 80: 
>> 81:   objArrayOop allocate(int length, TRAPS);
> 
> Do you think `multi_allocate` will need a better name in the future?

I thought if changing multi_allocate_instance so that it's clear that it's an instance, but decided to limit this.  Maybe this would be helpful but the allocate() function was the most confusing to me, that's why I picked that one.

> src/hotspot/share/runtime/reflection.cpp line 352:
> 
>> 350:     if (type == T_VOID) {
>> 351:       THROW_NULL(vmSymbols::java_lang_IllegalArgumentException());
>> 352:     }
> 
> I was first wondering where this came from but I now see that this was duplicated from `basic_type_mirror_to_arrayklass`. I wonder if this could could be deduplicated by moving this check into `basic_type_mirror_to_basic_type` and then removed from -`basic_type_mirror_to_arrayklass`:
> 
> 
> static BasicType basic_type_mirror_to_basic_type(oop basic_type_mirror, TRAPS) {
>   assert(java_lang_Class::is_primitive(basic_type_mirror),
>     "just checking");
>   
>   if (type == T_VOID) {
>     THROW_NULL(vmSymbols::java_lang_IllegalArgumentException());
>   }
> 
>   return java_lang_Class::primitive_type(basic_type_mirror);
> }
> 
> static Klass* basic_type_mirror_to_arrayklass(oop basic_type_mirror, TRAPS) {
>   BasicType type = basic_type_mirror_to_basic_type(basic_type_mirror, CHECK_NULL);
>   return Universe::typeArrayKlass(type);
> }
> 
> And then this code could be a two-liner again:
> 
> if (java_lang_Class::is_primitive(element_mirror)) {
>     BasicType type = basic_type_mirror_to_basic_type(element_mirror, CHECK_NULL);
>   return oopFactory::new_typeArray(type, length, CHECK_NULL);
> }

Unfortunately the caller to basic_type_mirror_to_basic_type() can legitimately return T_VOID for the caller in reflect_method, so that's why I had to duplicate the exception code.  Maybe a future enhancement would be to move these to javaClasses.hpp in java_lang_Class, where it knows all about is_primitive types, and boxing classes, which I guess boxing T_VOID is a thing.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/25590#discussion_r2135760802
PR Review Comment: https://git.openjdk.org/jdk/pull/25590#discussion_r2135763584
PR Review Comment: https://git.openjdk.org/jdk/pull/25590#discussion_r2135758517