On Mon, 25 May 2026 09:10:08 GMT, David Simms <[email protected]> wrote:
>> This is a "*sub-review pull request*" for the first >> [preview](https://openjdk.org/jeps/12) of [JEP 401: Value Classes and >> Objects](https://openjdk.org/jeps/401), specifically >> [JDK-8317278](https://bugs.openjdk.org/browse/JDK-8317278): JVM >> implementation of value classes and objects. >> >>> [!NOTE] >>> This pull request and the other sub-review pull requests listed below are >>> based on the "*master pull request*" >>> (https://github.com/openjdk/jdk/pull/31120). It contains the same full set >>> of code changes as the "*master pull request*" to preserve the full >>> implementation context; the language compiler, JVM, and standard library >>> changes are intertwined. This separate pull requests exist only to >>> subdivide the review and related discussion by area. >> >> Other areas for review: >> >> - [JDK-8317277](https://bugs.openjdk.org/browse/JDK-8317277): Java language >> implementation of value classes and objects >> - https://github.com/openjdk/jdk/pull/31121 >> - [JDK-8317279](https://bugs.openjdk.org/browse/JDK-8317279): Standard >> library implementation of value classes and objects >> - https://github.com/openjdk/jdk/pull/31123 >> >> Code changes resulting from the review process should be made in >> [`valhalla/lworld`](https://github.com/openjdk/valhalla/). >> >> `valhalla/lworld` is currently updated from `jdk/master` whenever a weekly >> [`jdk` tag](https://github.com/openjdk/jdk/tags) is created. At that time, >> code changes from `valhalla/lworld` will be propagated to the master pull >> request and to all sub-review pull requests, including this one. >> >> Ultimately, review sign-off will be recorded on the "*master pull request*", >> and this pull request will be closed without integration. >> >> This pull request has a large code surface area and often conflicts with >> `jdk/master` on a daily basis. Refer to >> [`valhalla/lworld`](https://github.com/openjdk/valhalla/) for the latest >> state of the project code, keeping in mind that it may lag several days >> behind `jdk/master`. Both repositories may be needed as references during >> review. >> >> --------- >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > David Simms has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 2732 commits: > > - Merge remote-tracking branch 'valhalla/lworld' into 8317277 > - 8385344: [lworld] ProblemList > tools/javac/platform/CanHandleClassFilesTest.java with --enable-preview > > Reviewed-by: fparain > - 8385301: [lworld] Remove > serviceability/sa/TestJhsdbJstackMixedWithXComp.java from problem list > > Reviewed-by: dsimms > - 8385331: [lworld] adjust ValueComparisonTest.java again to work around > JDK-8370769 > > Reviewed-by: dsimms > - 8385311: [lworld] TypePtr::eq() should use accessor method for _offset > > Reviewed-by: mchevalier > - 8385259: [lworld] Clean up LP64 in x86 code > > Reviewed-by: dlong, thartmann > - Merge > > Merge jdk-27+23 > - 8384924: [lworld] misc cleanups > > Reviewed-by: thartmann > - 8385167: [lworld] C1: minor cleanups > > Reviewed-by: dlong, thartmann > - 8384066: [lworld] TestDeadAllocationRemoval.java is ignored by jtreg > > Reviewed-by: thartmann > - ... and 2722 more: https://git.openjdk.org/jdk/compare/86637704...b3b4a2cb I have a few comments about changes in the oops directory, for which I have a cleanup patch. I also changed some /* */ comments to // comments per the coding standard. src/hotspot/share/oops/arrayOop.hpp line 80: > 78: static int arrayoopdesc_hs = 0; > 79: if (arrayoopdesc_hs == 0) arrayoopdesc_hs = hs; > 80: // assert(arrayoopdesc_hs == hs, "header size can't change"); Why is this commented out? src/hotspot/share/oops/instanceKlass.cpp line 575: > 573: > 574: #ifndef PRODUCT > 575: bool InstanceKlass::bounds_check(address addr, bool edge_ok, intptr_t > size_in_bytes) const { Should this be an assert instead of something that prints? src/hotspot/share/oops/instanceKlass.cpp line 3460: > 3458: } > 3459: > 3460: const char* InstanceKlass::signature_name_of_carrier(char c) const { This change isn't needed and looks like it's leftover from Q types (hint was the comment below). src/hotspot/share/oops/instanceKlass.cpp line 4093: > 4091: }; > 4092: > 4093: static void print_vtable(address self, intptr_t* start, int len, > outputStream* st) { I'm not sure what this is all about either. The 'self' parameter is always called with nullptr. This seems like it was left over from an earlier prototype with multiple entry points of an nmethod in the vtable, and for debugging? src/hotspot/share/oops/instanceKlass.cpp line 4186: > 4184: print_array_on(st, method_ordering(), [](outputStream* ost, int i) { > 4185: ost->print("%d", i); > 4186: }); We might want to revert this later. Printing the methods gets very verbose when printing the class, which is why it was under WizardMode/Verbose. Let's see if this becomes too bothersome. ------------- PR Review: https://git.openjdk.org/jdk/pull/31122#pullrequestreview-4375612261 PR Review Comment: https://git.openjdk.org/jdk/pull/31122#discussion_r3313752421 PR Review Comment: https://git.openjdk.org/jdk/pull/31122#discussion_r3313478445 PR Review Comment: https://git.openjdk.org/jdk/pull/31122#discussion_r3313573170 PR Review Comment: https://git.openjdk.org/jdk/pull/31122#discussion_r3313589177 PR Review Comment: https://git.openjdk.org/jdk/pull/31122#discussion_r3313641368
