On Fri, 13 Mar 2026 07:40:26 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 > > Stefan Karlsson has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 14 commits: > > - Merge remote-tracking branch 'valhalla/lworld' into > lworld_array_klass_cleanups > - Update assert in CollectedHeap::array_allocate > - Merge remote-tracking branch 'valhalla/lworld' into > lworld_array_klass_cleanups > - faKlass => fak > - Stray whitespaces > - Restore jniCheck.cpp > - Array cleanups > - Retype _objectArrayKlass > - allocate_instance > - Small cleanups > - ... and 4 more: > https://git.openjdk.org/valhalla/compare/db5c1873...531d7d01 Great improvement! Thanks for doing this. Just a few comments. src/hotspot/share/memory/oopFactory.cpp line 144: > 142: > 143: ArrayKlass* ak = klass->array_klass(CHECK_NULL); > 144: ObjArrayKlass* oak = > ObjArrayKlass::cast(ak)->klass_with_properties(props, CHECK_NULL); Potential follow-up. Evaluate changing the `Klass::array_klass` interface to return an `ObjArrayKlass*`, AFAIK we never get `TypeArrayKlass*` this way as we do not have `Klass*` for primitives. src/hotspot/share/memory/universe.cpp line 521: > 519: // Create a RefArrayKlass (which is the default) and initialize. > 520: ObjArrayKlass* rak = > ObjArrayKlass::cast(oak)->klass_with_properties(ArrayProperties::Default(), > THREAD); > 521: _objectArrayKlass = RefArrayKlass::cast(rak); Should this use `CHECK`. Clearly something is wrong if we fail this early, but in debug build we will crash inside `RefArrayKlass::cast`. Unclear why we used THREAD here at all as genesis will just return, hit an exception mark and exit the VM with an appropriate `vm_exit_during_initialization` message. src/hotspot/share/oops/flatArrayOop.inline.hpp line 123: > 121: faklass->oop_oop_iterate_elements_range<narrowOop>(this, blk, start, > end); > 122: } else { > 123: faklass->oop_oop_iterate_elements_range<oop>(this, blk, start, end); Potential follow-up: Would be nice to att covariant return types for `klass()` in our oopDesc hierarchy. So we would have `flatArrayKlass* flatArrayOopDesc::klass()` here and could just write `klass()->oop_oop_iterate_elements_range<...>(...)` src/hotspot/share/runtime/deoptimization.cpp line 1335: > 1333: FlatArrayKlass* ak = FlatArrayKlass::cast(k); > 1334: // Inline type array must be zeroed because not all memory is > reassigned > 1335: // FIXME: Is this missing an InternalOOMEMark? I think we should have a `InternalOOMEMark` here. We should neither end up in `report_java_out_of_memory` nor create a backtrace if we fail to allocate here. ------------- Changes requested by aboldtch (Committer). PR Review: https://git.openjdk.org/valhalla/pull/2207#pullrequestreview-3942307176 PR Review Comment: https://git.openjdk.org/valhalla/pull/2207#discussion_r2929664462 PR Review Comment: https://git.openjdk.org/valhalla/pull/2207#discussion_r2929684403 PR Review Comment: https://git.openjdk.org/valhalla/pull/2207#discussion_r2929717554 PR Review Comment: https://git.openjdk.org/valhalla/pull/2207#discussion_r2929748005
