[lworld] RFR: 8375306: [lworld] Investigate alternatives to flatArrayOopDesc::obj_at implementation [v2]

Frederic Parain fparain at openjdk.org
Mon Feb 9 22:03:34 UTC 2026 

On Mon, 9 Feb 2026 09:39:00 GMT, Stefan Karlsson <stefank at openjdk.org> wrote:

>> Frederic Parain has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 12 additional commits since the last revision:
>> 
>>  - Comments update
>>  - Rename new_default_refArray to new_refArray with overload
>>  - Fixes issues mentioned in reviews
>>  - Remove force_refarray and add array klass creation from ArrayDescription
>>  - Fix merge
>>  - Merge remote-tracking branch 'upstream/lworld' into refarray_factory
>>  - Revert foreign methods signature changes
>>  - Foreign API and other fixes
>>  - CI fixes
>>  - More fixes in serviceability code
>>  - ... and 2 more: https://git.openjdk.org/valhalla/compare/dbed07f5...bd5f33d0
>
> src/hotspot/share/memory/oopFactory.cpp line 135:
> 
>> 133: }
>> 134: 
>> 135: 
> 
> Suggestion:
> 
> }

Fixed

> src/hotspot/share/memory/oopFactory.hpp line 63:
> 
>> 61:   // Factory forcing the creation of a reference array
>> 62:   static refArrayOop     new_refArray(Klass* klass, int length, ArrayKlass::ArrayProperties properties, TRAPS);
>> 63:   static refArrayOop     new_default_refArray(Klass* klass, int length, TRAPS);
> 
> Swap order to match the block above (alt. swap the order above).
> Suggestion:
> 
>   static refArrayOop     new_default_refArray(Klass* klass, int length, TRAPS);
>   static refArrayOop     new_refArray(Klass* klass, int length, ArrayKlass::ArrayProperties properties, TRAPS);

Fixed

> src/hotspot/share/oops/flatArrayOop.hpp line 43:
> 
>> 41:   void* value_at_addr(int index, jint lh) const;
>> 42: 
>> 43:   inline static flatArrayOop cast(oop o);
> 
> I wonder when we should have these *OopDesc casts and when we should use the oopCast.hpp functions?

This is a good question, I've created JDK-8377466 to track this issue and address it in a different patch.

> src/hotspot/share/oops/flatArrayOop.inline.hpp line 59:
> 
>> 57: 
>> 58: inline oop flatArrayOopDesc::obj_at(int index) const {
>> 59:   fatal("Should not reach here");
> 
> Maybe:
> Suggestion:
> 
>   fatal("Should not be used with flat arrays");

Fixed.

> src/hotspot/share/oops/klass.hpp line 61:
> 
>> 59: class PackageEntry;
>> 60: class vtableEntry;
>> 61: class ArrayDescription;
> 
> Would be nice if you inserted this sorted.

Fixed.

> src/hotspot/share/oops/klass.hpp line 625:
> 
>> 623: #ifdef ASSERT
>> 624:   void validate_array_description(ArrayDescription ad);
>> 625: #endif // ASSERT
> 
> Suggestion:
> 
>   void validate_array_description(ArrayDescription ad) NOT_DEBUG_RETURN;
> 
> 
> We also tend to try to keep verification code at the end of the oops/ classes, so maybe it would be good too place it there as well.

Fixed

> src/hotspot/share/oops/objArrayKlass.cpp line 181:
> 
>> 179:   // TODO FIXME: the layout selection should take the array size in consideration
>> 180:   // to avoid creation of arrays too big to be handled by the VM. See JDK-8233189
>> 181:   if (!UseArrayFlattening || element->is_array_klass() || element->is_identity_class()|| element->is_abstract()) {
> 
> Suggestion:
> 
>   if (!UseArrayFlattening || element->is_array_klass() || element->is_identity_class() || element->is_abstract()) {

Fixed

> src/hotspot/share/oops/objArrayKlass.cpp line 412:
> 
>> 410: }
>> 411: 
>> 412: ObjArrayKlass* ObjArrayKlass::klass_from_description(ArrayDescription adesc, TRAPS) {
> 
> `adesc` is some sort of a mix because an abbreviation and a shortening of a word, and it is unconventional in our code base. Maybe just use `ad` for it and change the other `ad` to a name with a more explicit name that indicates why you need to figure out a new `ArrayDescription`?

Fixed

> src/hotspot/share/oops/objArrayKlass.cpp line 416:
> 
>> 414: #ifdef ASSERT
>> 415:   element_klass()->validate_array_description(adesc);
>> 416: #endif // ASSERT
> 
> If you took the proposed `NOT_DEBUG_RETURN` change then this could be changed to:
> Suggestion:
> 
>   element_klass()->validate_array_description(adesc);

Fixed.

> src/hotspot/share/oops/objArrayKlass.cpp line 439:
> 
>> 437:         release_set_next_refined_klass(first);
>> 438:       }
>> 439:       ak = allocate_klass_from_description(adesc, THREAD);
> 
> Pre-existing but still weird: `ak` is set here and then unconditionally overwritten further down. I'd suggest that you use another local variable to hold this klass.

Fixed.

> src/hotspot/share/opto/runtime.cpp line 369:
> 
>> 367:       // Null-free arrays need to be initialized
>> 368: #ifdef ASSERT
>> 369:       ObjArrayKlass* oak = ObjArrayKlass::cast(result->klass());
> 
> `oak` shadows the earlier `oak`, which makes the code harder to read.

Fixed

> src/hotspot/share/opto/runtime.cpp line 372:
> 
>> 370:       assert(oak->is_null_free_array_klass(), "Sanity check");
>> 371: #endif
>> 372:       // assert(!h_init_val.is_null(), "Cannot initialize null free array with null");
> 
> This was already checked above
> Suggestion:

Removed.

> src/hotspot/share/prims/jvm.cpp line 1452:
> 
>> 1450: 
>> 1451:   // Allocate temp. result array
>> 1452:   refArrayOop r = oopFactory::new_default_refArray(vmClasses::Class_klass(), length/4, CHECK_NULL);
> 
> Suggestion:
> 
>   refArrayOop r = oopFactory::new_default_refArray(vmClasses::Class_klass(), length / 4, CHECK_NULL);

Fixed

> src/hotspot/share/prims/jvm.cpp line 1453:
> 
>> 1451:   // Allocate temp. result array
>> 1452:   refArrayOop r = oopFactory::new_default_refArray(vmClasses::Class_klass(), length/4, CHECK_NULL);
>> 1453:   refArrayHandle result (THREAD, r);
> 
> Suggestion:
> 
>   refArrayHandle result(THREAD, r);

Fixed

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

PR Review Comment: https://git.openjdk.org/valhalla/pull/2033#discussion_r2782486515
PR Review Comment: https://git.openjdk.org/valhalla/pull/2033#discussion_r2782543407
PR Review Comment: https://git.openjdk.org/valhalla/pull/2033#discussion_r2782597513
PR Review Comment: https://git.openjdk.org/valhalla/pull/2033#discussion_r2782552634
PR Review Comment: https://git.openjdk.org/valhalla/pull/2033#discussion_r2782604185
PR Review Comment: https://git.openjdk.org/valhalla/pull/2033#discussion_r2782612792
PR Review Comment: https://git.openjdk.org/valhalla/pull/2033#discussion_r2782640463
PR Review Comment: https://git.openjdk.org/valhalla/pull/2033#discussion_r2782789820
PR Review Comment: https://git.openjdk.org/valhalla/pull/2033#discussion_r2782644584
PR Review Comment: https://git.openjdk.org/valhalla/pull/2033#discussion_r2782742333
PR Review Comment: https://git.openjdk.org/valhalla/pull/2033#discussion_r2782766509
PR Review Comment: https://git.openjdk.org/valhalla/pull/2033#discussion_r2782767677
PR Review Comment: https://git.openjdk.org/valhalla/pull/2033#discussion_r2782777793
PR Review Comment: https://git.openjdk.org/valhalla/pull/2033#discussion_r2782783230

More information about the valhalla-dev mailing list