On Mon, 9 Mar 2026 12:33:38 GMT, Stefan Karlsson <[email protected]> wrote:
> I've been looking over the current state of ArrayKlass and the sub-classes
> and made various cleanups and simplifications that I'd like to get integrated:
>
> * Type Universe::_objectArrayKlass as RefArrayKlass
> * Remove dead flat array code in code in javaClasses.cpp
> * Used is_refined_objArray_klass where appropriate
> * Introduce is_unrefined_objArray for asserts and checks
> * Restore code and whitespace changes compared to upstream
> * Renamed faklass to fak in oops/ and GC code (I didn't touch other areas
> that used that name)
> * Devirtualized ObjArrayKlass::allocate_instance and simplified related code
> * Moved ArrayKlass::_properties to after the variables for array dimensions.
> * Made ArrayKlass::_properties const and non-settable
> * Unified oop_iterate_elements_range implementations
> * Added ShouldNotReachHere implementation of ObjArrayKlass::copy_array
> * Removed redundant check in jniCheck.cpp and restored the file
src/hotspot/share/classfile/javaClasses.cpp line 1124:
> 1122: comp_mirror = Handle(THREAD,
> HeapShared::scratch_java_mirror(element_klass));
> 1123: } else {
> 1124: comp_mirror = Handle(THREAD, element_klass->java_mirror());
This restore diff against jdk/jdk
src/hotspot/share/gc/shared/collectedHeap.inline.hpp line 42:
> 40:
> 41: inline oop CollectedHeap::array_allocate(Klass* klass, size_t size, int
> length, bool do_zero, TRAPS) {
> 42: assert(klass->is_typeArray_klass() ||
> klass->is_refined_objArray_klass(), "ObjArrayKlass must never be used to
> allocate array instances directly");
Could probably also be changed to:
Suggestion:
assert(!klass->is_unrefined_objArray_klass(), "ObjArrayKlass must never be
used to allocate array instances directly");
src/hotspot/share/memory/universe.cpp line 398:
> 396: InstanceKlass::cast(k)->restore_unshareable_info(loader_data,
> Handle(), nullptr, CHECK);
> 397: } else {
> 398: TypeArrayKlass::cast(k)->restore_unshareable_info(loader_data,
> Handle(), CHECK);
CDS has some peculiarities with `remove_unshareable_info` being virtual and
`restore_unshareable_info` not being virtual. With lworld there's new code in
`ObjArrayKlass` and if one calls `ArrayKlass` on an `ObjArrayKlass`, the wrong
version will be called. I've changed this to `TypeArrayKlass` to make it clear
that we are not calling the somewhat dangerous `ArrayKlass` version.
src/hotspot/share/oops/arrayKlass.cpp line 172:
> 170: // Create multi-dim klass object and link them together
> 171: ObjArrayKlass* ak =
> 172: ObjArrayKlass::allocate_objArray_klass(class_loader_data(),
> dim + 1, this, CHECK_NULL);
Restored diff against jdk/jdk.
src/hotspot/share/oops/objArrayKlass.cpp line 268:
> 266: ObjArrayKlass* oak = klass_with_properties(ArrayProperties::Default(),
> CHECK_NULL);
> 267: assert(oak->is_refArray_klass() || oak->is_flatArray_klass(), "Must
> be");
> 268: objArrayOop array = oak->allocate_instance(length,
> ArrayProperties::Default(), CHECK_NULL);
This code was a bit awkward. First a set of properties was used to get an
`ObjArrayKlass` and then another set of properties (but the same value) was
pass down into `allocate_instance`, which also called klass_with_properties.
That's why the `klass_with_properties` call isn't needed here.
src/hotspot/share/oops/objArrayKlass.cpp line 285:
> 283: int dst_pos, int length, TRAPS) {
> 284: ShouldNotReachHere();
> 285: }
This catches if someone tries to copy via the unrefined ObjArrayKlass instance.
src/hotspot/share/oops/objArrayKlass.cpp line 487:
> 485: guarantee(bottom_klass()->is_klass(), "should be klass");
> 486: Klass* bk = bottom_klass();
> 487: guarantee(bk->is_instance_klass() || bk->is_typeArray_klass() ||
> bk->is_flatArray_klass(),
This part of the assert is wrong.
src/hotspot/share/oops/objArrayOop.inline.hpp line 48:
> 46: assert(is_within_bounds(index), "index %d out of bounds %d", index,
> length());
> 47: if (is_flatArray()) {
> 48: return ((const flatArrayOopDesc* )this)->obj_at(index);
We really should not be calling this version for flat arrays. One more step
towards removing that version of flatArrayOopDec::obj_at.
src/hotspot/share/oops/refArrayKlass.hpp line 68:
> 66:
> 67: public:
> 68: // Copying TODO FIXME make copying method in objArrayKlass virtual and
> default implementation invalid (ShouldNotReachHere())
Done
src/hotspot/share/oops/refArrayOop.cpp line 42:
> 40: Klass* refArrayOopDesc::element_klass() {
> 41: return RefArrayKlass::cast(klass())->element_klass();
> 42: }
We already have a getter in `objArrayOopDesc`.
src/hotspot/share/oops/refArrayOop.hpp line 32:
> 30:
> 31: #include <type_traits>
> 32:
Should not be used anymore now that we have wrapper headers for this. The check
that needs this is a duplicate of whats in the super class header. I figured
that we could just rely on that include, but we could also make it extra
explicit and include it in these headers as well.
-------------
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2207#discussion_r2905154218
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2207#discussion_r2905158805
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2207#discussion_r2905186596
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2207#discussion_r2905190251
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2207#discussion_r2905205114
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2207#discussion_r2905207674
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2207#discussion_r2905210127
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2207#discussion_r2905217058
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2207#discussion_r2905220542
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2207#discussion_r2905223358
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2207#discussion_r2905234702
