On Mon, 9 Feb 2026 09:39:00 GMT, Stefan Karlsson <[email protected]> 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
