On Wed, 3 Sep 2025 14:43:55 GMT, Frederic Parain <[email protected]> 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
