[lworld] RFR: 8366705: [lworld] Re-work of arrays meta-data [v4]
Coleen Phillimore
coleenp at openjdk.org
Wed Sep 3 18:04:06 UTC 2025
On Wed, 3 Sep 2025 14:43:55 GMT, Frederic Parain <fparain at openjdk.org> wrote:
>> Since the removal of Q-types and the notion that nullability was not part of the Java type, there was an awkward situation because nullable arrays of value types and null free arrays of value types had each a different Java mirror when they were in fact supposed to have the same Java type.
>> In order to accommodate to the new situation, that arrays can have properties (nullability, flatness, atomicity, etc.) that are not part of their Java type, the 1-1 relationship between the *ArrayKlass and the Java mirror must be broken.
>> The proposed solution is to dedicate one instance of ObjArrayKlass to represent the Java type of the array in the JVM, and have this instance being the counterpart of the Java mirror of the array, and have several instances of RefArrayKlass and FlatArrayKlass that represent the refinements of the Java array type. Each RefArrayKlass/FlatArrayKlass encodes the characteristic of a Java array for a given element type and a set of properties.
>
> Frederic Parain has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 67 commits:
>
> - Merge branch 'array_klasses' of github.com:fparain/valhalla into array_klasses
> - Forgot a TODO
> - Small cleanup
> - Merge remote-tracking branch 'upstream/lworld' into array_klasses
> - Moved get_Klass() back to protected and updated usages
> - Merge branch 'array_klasses' of github.com:fparain/valhalla into array_klasses
> - Linked TODOs to JDK-8366668
> - Multidim array fix
> - Cleanup T_FLAT_ELEMENT related code
> - Fix for isAssignableFrom + tests
> - ... and 57 more: https://git.openjdk.org/valhalla/compare/22e9d5f5...527a17b6
I reviewed the runtime/oops/interpreter code. My comments are questions and very minor suggestions, if you want to do them now. This is a very nice cleanup of the existing lworld repo array code.
make/test/BuildMicrobenchmark.gmk line 97:
> 95: --add-exports java.base/jdk.internal.misc=ALL-UNNAMED \
> 96: --add-exports java.base/jdk.internal.util=ALL-UNNAMED \
> 97: --add-exports java.base/jdk.internal.value=ALL-UNNAMED \
Indentation? I hope I got the number of spaces right.
Suggestion:
--add-exports java.base/jdk.internal.value=ALL-UNNAMED \
src/hotspot/share/cds/cdsProtectionDomain.cpp line 300:
> 298: // It doesn't matter which racing thread wins, as long as only one
> 299: // result is used by all threads, and all future queries.
> 300: // ((objArrayOop)array.resolve())->replace_if_null(index, o);
Delete the commented out line.
src/hotspot/share/cds/dynamicArchive.cpp line 371:
> 369: if (oak->is_refined_objArray_klass()) {
> 370: oak = ObjArrayKlass::cast(oak->super());
> 371: }
Why is this needed?
src/hotspot/share/cds/heapShared.cpp line 1299:
> 1297: Klass* resolved_k = SystemDictionary::resolve_or_null(k->name(), CHECK);
> 1298: if (resolved_k->is_array_klass()) {
> 1299: assert(resolved_k == k || ((ObjArrayKlass*)resolved_k)->next_refined_array_klass() == k, "classes used by archived heap must not be replaced by JVMTI ClassFileLoadHook");
Should this be is_objArray_klass() ? and ObjArrayKlass::cast(resolved_k) instead?
src/hotspot/share/classfile/javaClasses.cpp line 1137:
> 1135: if (vmClasses::Class_klass_loaded()) {
> 1136:
> 1137: if (k->is_refArray_klass() || k->is_flatArray_klass()) {
There was a is_refined_objArray_klass() method added that I think can be used here.
src/hotspot/share/classfile/javaClasses.cpp line 1246:
> 1244: } else {
> 1245: ObjArrayKlass* objarray_k = (ObjArrayKlass*)as_Klass(m);
> 1246: // Mirror is either an ObjArrayKlass or one of its refined array klasses
This is a confusing comment. Refined array klasses don't have their own mirrors. Maybe:
Suggestion:
// Mirror should be restored for an ObjArrayKlass or one of its refined array klasses.
src/hotspot/share/classfile/javaClasses.cpp line 1485:
> 1483: void java_lang_Class::release_set_array_klass(oop java_class, Klass* klass) {
> 1484: assert(klass->is_klass() && klass->is_array_klass(), "should be array klass");
> 1485: assert(!klass->is_refArray_klass() && !klass->is_flatArray_klass(), "should not be ref or flat array klass");
Suggestion:
assert(!klass->is_refined_objArray_klass(), "should not be ref or flat array klass");
Hope that's right.
src/hotspot/share/jfr/jni/jfrUpcalls.cpp line 271:
> 269:
> 270: // new String[method_count]
> 271: objArrayOop signature_array = oopFactory::new_objArray(vmClasses::String_klass(), method_count, ArrayKlass::ArrayProperties::DEFAULT, CHECK_NULL);
I wonder if there are enough of these to create a default ref array to overload oopFactory::new_objArray() for DEFAULT.
src/hotspot/share/memory/oopFactory.cpp line 140:
> 138:
> 139: objArrayHandle oopFactory::new_objArray_handle(Klass* klass, int length, TRAPS) {
> 140: // TODO FIXME check if this method should take an array properties argument (probably should)
I was thinking the overload to have the default argument be DEFAULT is good. Maybe we should remove new_objArray_handle in mainline though. There aren't that many.
src/hotspot/share/memory/universe.hpp line 32:
> 30: #include "oops/array.hpp"
> 31: #include "oops/oopHandle.hpp"
> 32: #include "oops/refArrayKlass.hpp"
Should be included in the .cpp file instead? We could fix this later though.
src/hotspot/share/oops/arrayKlass.cpp line 174:
> 172: // Create multi-dim klass object and link them together
> 173: ObjArrayKlass* ak = nullptr;
> 174: ak = RefArrayKlass::allocate_objArray_klass(class_loader_data(), dim + 1, this, CHECK_NULL);
Can you make this one line?
src/hotspot/share/oops/constantPool.cpp line 196:
> 194: oop ConstantPool::set_resolved_reference_at(int index, oop new_result) {
> 195: assert(oopDesc::is_oop_or_null(new_result), "Must be oop");
> 196: // return resolved_references()->replace_if_null(index, new_result);
Can remove the commented out line.
src/hotspot/share/oops/klass.hpp line 473:
> 471: static const unsigned int _lh_array_tag_type_value = 0Xfffffffc;
> 472: static const unsigned int _lh_array_tag_flat_value = 0Xfffffffa;
> 473: static const unsigned int _lh_array_tag_ref_value = 0Xfffffff8;
So there is no more layout_helper value for objArray, right? Only flat and ref values?
src/hotspot/share/oops/objArrayKlass.cpp line 122:
> 120: bk = ObjArrayKlass::cast(element_klass)->bottom_klass();
> 121: } else if (element_klass->is_flatArray_klass()) {
> 122: bk = FlatArrayKlass::cast(element_klass)->element_klass(); // flat array case should be merge with refArray case once reparented
TODO? Not sure what this means. FlatArrayKlass inherits from ObjArrayKlass in your patch, so this is done?
src/hotspot/share/oops/refArrayKlass.hpp line 77:
> 75:
> 76: public:
> 77: static RefArrayKlass *cast(Klass *k) {
Should the cast function supposed to have an precond(is_refArray_klass()); before casting?
src/hotspot/share/oops/typeArrayKlass.cpp line 41:
> 39: #include "oops/typeArrayKlass.inline.hpp"
> 40: #include "oops/typeArrayOop.inline.hpp"
> 41: #include "runtime/arguments.hpp"
Is this include needed?
src/hotspot/share/prims/jni.cpp line 2400:
> 2398: oop v = JNIHandles::resolve(value);
> 2399: if (a->is_within_bounds(index)) {
> 2400: // TODO FIXME Temporary hack, to be removed when FlatArrayKlass is made a sub-class of ObjArrayKlass
You've done this. Can this be cleaned up here?
-------------
Marked as reviewed by coleenp (Committer).
PR Review: https://git.openjdk.org/valhalla/pull/1452#pullrequestreview-3181368314
PR Review Comment: https://git.openjdk.org/valhalla/pull/1452#discussion_r2319461654
PR Review Comment: https://git.openjdk.org/valhalla/pull/1452#discussion_r2319478984
PR Review Comment: https://git.openjdk.org/valhalla/pull/1452#discussion_r2319484976
PR Review Comment: https://git.openjdk.org/valhalla/pull/1452#discussion_r2319493987
PR Review Comment: https://git.openjdk.org/valhalla/pull/1452#discussion_r2319498964
PR Review Comment: https://git.openjdk.org/valhalla/pull/1452#discussion_r2319504994
PR Review Comment: https://git.openjdk.org/valhalla/pull/1452#discussion_r2319511716
PR Review Comment: https://git.openjdk.org/valhalla/pull/1452#discussion_r2319522187
PR Review Comment: https://git.openjdk.org/valhalla/pull/1452#discussion_r2319529334
PR Review Comment: https://git.openjdk.org/valhalla/pull/1452#discussion_r2319533046
PR Review Comment: https://git.openjdk.org/valhalla/pull/1452#discussion_r2319536052
PR Review Comment: https://git.openjdk.org/valhalla/pull/1452#discussion_r2319540618
PR Review Comment: https://git.openjdk.org/valhalla/pull/1452#discussion_r2319583320
PR Review Comment: https://git.openjdk.org/valhalla/pull/1452#discussion_r2319592380
PR Review Comment: https://git.openjdk.org/valhalla/pull/1452#discussion_r2319740381
PR Review Comment: https://git.openjdk.org/valhalla/pull/1452#discussion_r2319745779
PR Review Comment: https://git.openjdk.org/valhalla/pull/1452#discussion_r2319751308
More information about the valhalla-dev
mailing list