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

Mon Feb 9 22:03:13 UTC 2026

On Mon, 9 Feb 2026 19:47:00 GMT, Frederic Parain <fparain at openjdk.org> wrote:

>> First batch of changes to remove potentially dangerous calls to objArrayOopDesc::obj_at().
>> Changes are more extensive than intended. In most cases, code modifications consist in using a refArrayOop type instead of a objArrayOop type, because most of the arrays the JVM deals with for its own purpose are always reference arrays (because they are arrays of identity type elements). The patch also adds a new API allowing the VM to request the allocation of a reference array.
>> Code dealing with user provided arrays must be ready to handle exceptions when accessing objArrays.
>> 
>> This is a short term fix, fixing a few bugs, and trying to make the code more robust using the meta-data types. For the long term, a better solution is needed. Accesses to both arrays and fields are becoming more and more complex because of the introduction of flattening, multiple layouts, additional properties. Forcing enforcement at each access would be expensive and wasteful, as the JVM usually operates on well-known objects or arrays. But because of the increasing complexity, having a way to quickly check the validity of an access would help making the VM code more robust.
>
> 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

Given that "default" ref arrays is going to be the most common type of array we create in the runtime code I would suggest that you revert the name `oopFactory::new_default_refArray` to nicer `oopFactory::new_refArray` name, and then we can have a longer name for the when we need to add some extra properties (E.g. oopFactory::new_refArray_with_properties`).

I've added comments about nits below.

src/hotspot/share/memory/oopFactory.cpp line 135:

> 133: }
> 134: 
> 135: 

Suggestion:

}

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);

src/hotspot/share/memory/universe.hpp line 132:

> 130: 
> 131:   // the array of preallocated errors with backtraces
> 132:   static refArrayOop  preallocated_out_of_memory_errors();

While your at it ...
Suggestion:

  static refArrayOop preallocated_out_of_memory_errors();

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?

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");

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.

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.

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()) {

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`?

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);

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.

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.

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:

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);

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);

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

PR Review: https://git.openjdk.org/valhalla/pull/2033#pullrequestreview-3772241338
PR Review Comment: https://git.openjdk.org/valhalla/pull/2033#discussion_r2781611025
PR Review Comment: https://git.openjdk.org/valhalla/pull/2033#discussion_r2781615775
PR Review Comment: https://git.openjdk.org/valhalla/pull/2033#discussion_r2781619958
PR Review Comment: https://git.openjdk.org/valhalla/pull/2033#discussion_r2781638411
PR Review Comment: https://git.openjdk.org/valhalla/pull/2033#discussion_r2781642310
PR Review Comment: https://git.openjdk.org/valhalla/pull/2033#discussion_r2781649379
PR Review Comment: https://git.openjdk.org/valhalla/pull/2033#discussion_r2781667160
PR Review Comment: https://git.openjdk.org/valhalla/pull/2033#discussion_r2781668703
PR Review Comment: https://git.openjdk.org/valhalla/pull/2033#discussion_r2781707595
PR Review Comment: https://git.openjdk.org/valhalla/pull/2033#discussion_r2781680376
PR Review Comment: https://git.openjdk.org/valhalla/pull/2033#discussion_r2781744425
PR Review Comment: https://git.openjdk.org/valhalla/pull/2033#discussion_r2781762480
PR Review Comment: https://git.openjdk.org/valhalla/pull/2033#discussion_r2781765171
PR Review Comment: https://git.openjdk.org/valhalla/pull/2033#discussion_r2781771456
PR Review Comment: https://git.openjdk.org/valhalla/pull/2033#discussion_r2781772506