On Tue, 16 Jun 2026 12:13:25 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-8317279](https://bugs.openjdk.org/browse/JDK-8317279): Standard library >> 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-8317278](https://bugs.openjdk.org/browse/JDK-8317278): JVM >> implementation of value classes and objects >> - https://github.com/openjdk/jdk/pull/31122 >> >> 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. >> >> # PR implementation description >> >> Here's a high-level overview of what's included here. >> >> ### Core object behaviors >> >> Introduced `Objects` methods to test for identity and `IdentityException` for >> test failures; revised definitions of `==` and `identityHashCode` to work on >> the >> fields of value objects. >> >> - `src/java.base/share/classes/java/lang` >> - `src/java.base/share/classes/java/util` >> - `src/java.base/share/classes/java/lang/runtime` >> - ... > > David Simms has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 2822 commits: > > - Merge branch '8317277' into jep401_sub_review_8317279 > - Merge remote-tracking branch 'valhalla/lworld' into 8317277 > - 8386690: [lworld] cherry-pick JDK-8386124 Test > serviceability/sa/TestG1HeapRegion.java failed: Address of G1HeapRegion does > not match > > Reviewed-by: liach > - 8385743: [lworld] investigate and address build related comments in the > Valhalla PR > > Reviewed-by: liach, erikj > - 8376346: [lworld] Basic Shenandoah support > > Reviewed-by: qamai > - 8386598: [lworld] C1 acmp profiling fix and minor cleanup > > Reviewed-by: mchevalier > - 8386618: [lworld] Remove unused entry_guard_Relocation > > Reviewed-by: chagedorn, jsjolen, jsikstro > - Merge > > Merge jdk-28+2 > - 8386623: [lworld] Cleanup module support for system modules in JDK > exploded build with preview resources > > Reviewed-by: liach > - 8386316: [lworld] Replace Valhalla since version with 28 > > Reviewed-by: darcy > - ... and 2812 more: https://git.openjdk.org/jdk/compare/6def7d55...426cc065 Feedback from my latest pass over java.base changes in this PR src/java.base/share/classes/java/io/Externalizable.java line 2: > 1: /* > 2: * Copyright (c) 1996, 2020, Oracle and/or its affiliates. All rights > reserved. It has likely been stated before, but unless it hasn't, this file (and others) needs a copyright year update (go through the changed files and ensure that their copyright year reflects 2026). src/java.base/share/classes/java/io/ObjectStreamClass.java line 125: > 123: /** true if represents record type */ > 124: private boolean isRecord; > 125: /** true if represented class cannot use allocate-and-fill > deserialization, I think "when" reads better here, since we're talking about "true when negative condition". Suggestion: /** true when represented class cannot use allocate-and-fill deserialization, src/java.base/share/classes/java/io/ObjectStreamClass.java line 1357: > 1355: return; > 1356: exec.setAccessible(true); > 1357: sink.accept(exec); Seems cleaner to work off-of a positive rather than a negative: Suggestion: .<Executable>mapMulti((exec, sink) -> { if (isDeserializer(exec, fields)) { exec.setAccessible(true); sink.accept(exec); } src/java.base/share/classes/java/io/ObjectStreamClass.java line 2362: > 2360: var types = > Arrays.stream(recordComponents).map(RecordComponent::getType).toArray(Class<?>[]::new); > 2361: var names = > Arrays.stream(recordComponents).map(RecordComponent::getName).toArray(String[]::new); > 2362: int count = recordComponents.length; It's possible to avoid the creation of the two prototype arrays and having to make a two-pass over `recordComponents` since we know the arity ahead of time: var componentCount = recordComponents.length; var types = new Class<?>[componentCount; var names = new String[componentCount]; for(int i = 0;i < componentCount; ++i) { var rc = recordComponents[i]; types[i] = rc.getType(); names[i] = rc.getName(); } src/java.base/share/classes/java/lang/Class.java line 354: > 352: } > 353: if (isRecord()) { > 354: sb.append("record"); What about the scenario where it is a value record? src/java.base/share/classes/java/lang/Class.java line 645: > 643: } else { > 644: int mask = ClassFile.ACC_IDENTITY | ClassFile.ACC_INTERFACE; > 645: return !primitive && (getModifiers() & mask) == 0; To align with the other "getters" below, it seems like declaring the mask inline is the thing to do Suggestion: return !primitive && (getModifiers() & (ClassFile.ACC_IDENTITY | ClassFile.ACC_INTERFACE)) == 0; src/java.base/share/classes/java/lang/IdentityException.java line 55: > 53: public IdentityException(Class<?> clazz) { > 54: super(clazz.getName() + " is not an identity class"); > 55: } Is this constructor strictly necessary? src/java.base/share/classes/java/lang/Object.java line 43: > 41: * Use of value class instances for synchronization, mutexes, or > with > 42: * {@linkplain java.lang.ref.Reference object references} > results in > 43: * {@link IdentityException}. This sentence reads a bit ambiguously for me. "value class instances", does that refer to Class<?> or instances of value classes? Since I presume the latter, it would be an improvement to state "value class instances" instead. Secondly, and I believe this has been discussed before, is that using "mutexes" in the sentence is superfluous since "synchronization" should cover it. src/java.base/share/classes/java/lang/Short.java line 57: > 55: * <p>This is a <a > href="{@docRoot}/java.base/java/lang/doc-files/ValueBased.html">value-based</a> > 56: * class; programmers should treat instances that are {@linkplain > #equals(Object) equal} > 57: * as interchangeable and should not use instances for synchronization, > mutexes, or One thoughts as I was reviewing: does "mutexes" here refer to CAS/CAE `expected`-values? If that is the case, and I got confused, I think that it will be confusing to others as well. src/java.base/share/classes/java/lang/doc-files/ValueBased.html line 72: > 70: </p> > 71: <ul> > 72: <li>The class may choose to allocate/cache instances differently. This list item is a bit vague—does the *class* choose different, or does the runtime choose different? src/java.base/share/classes/java/lang/invoke/ArrayVarHandle.java line 36: > 34: import static java.lang.invoke.MethodHandleStatics.UNSAFE; > 35: > 36: /// The var handle for polymorphic arrays. Just a thought—this class might benefit from a @TrustFinalFields (needs to be measured of course) src/java.base/share/classes/java/lang/invoke/DirectMethodHandle.java line 941: > 939: if (!isGetter) { > 940: outArgs[x] = (needsCast ? names[PRE_CAST] : > names[SET_VALUE]); > 941: } This code, esp. the indexing into outArgs looks a bit fragile. I'd suggest either going with static indexing or using `x` throughput. Right now it is a bit of a mix which reads a bit harder than it should. src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 443: > 441: * classes that are referenced by the proxy interface have already > 442: * been loaded before the proxy class. Hence the proxy class is > 443: * generated with no loadable descriptors attributes as it > essentially has no effect. "essentially no effect" sounds purposeful and vague at the same time. Does that part add any value to the documentation? src/java.base/share/classes/jdk/internal/classfile/impl/StackMapGenerator.java line 33: > 31: import java.lang.classfile.Label; > 32: import java.lang.classfile.attribute.StackMapTableAttribute; > 33: import java.lang.classfile.constantpool.*; Is the *-import intentional? src/java.base/share/classes/jdk/internal/classfile/impl/WritableField.java line 2: > 1: /* > 2: * Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved. Copyright year still true? src/java.base/share/classes/jdk/internal/jrtfs/ExplodedImage.java line 389: > 387: packageToModules > 388: .computeIfAbsent(pkgName, k -> new > ArrayList<>()) > 389: .add(moduleName)); Excessive indententation? src/java.base/share/classes/jdk/internal/misc/Unsafe.java line 272: > 270: private native int nullMarkerOffset0(Object o); > 271: > 272: public static final int NON_FLAT_LAYOUT = 0; Might be good to have a comment to this constant, where it is defined (i.e. what specification stipulates its value) src/java.base/share/classes/jdk/internal/misc/Unsafe.java line 1743: > 1741: } > 1742: } > 1743: } else { Is this conditional verified to be dead-code-eliminated if the program isn't in preview mode? Otherwise I could see this causing performance regressions. src/java.base/share/classes/jdk/internal/misc/Unsafe.java line 1805: > 1803: V > expected, > 1804: V x) { > 1805: return compareAndExchangeReference(o, offset, valueType, > expected, x); I think all of these "weaker accesses" deserve an implementation comment stating why the implementation is much stronger than requested. src/java.base/share/classes/jdk/internal/misc/Unsafe.java line 2921: > 2919: putFlatValue(array, base + scale, layout, valueType, x); // put > x as object > 2920: switch (scale) { > 2921: case 1: { Might be better to break out the individual brenches into their own sub-methods as this method is ForceInline. ------------- PR Review: https://git.openjdk.org/jdk/pull/31123#pullrequestreview-4514950608 PR Review Comment: https://git.openjdk.org/jdk/pull/31123#discussion_r3427633449 PR Review Comment: https://git.openjdk.org/jdk/pull/31123#discussion_r3459185499 PR Review Comment: https://git.openjdk.org/jdk/pull/31123#discussion_r3459319361 PR Review Comment: https://git.openjdk.org/jdk/pull/31123#discussion_r3459390628 PR Review Comment: https://git.openjdk.org/jdk/pull/31123#discussion_r3462265451 PR Review Comment: https://git.openjdk.org/jdk/pull/31123#discussion_r3462282478 PR Review Comment: https://git.openjdk.org/jdk/pull/31123#discussion_r3462320942 PR Review Comment: https://git.openjdk.org/jdk/pull/31123#discussion_r3466219046 PR Review Comment: https://git.openjdk.org/jdk/pull/31123#discussion_r3466263586 PR Review Comment: https://git.openjdk.org/jdk/pull/31123#discussion_r3460214336 PR Review Comment: https://git.openjdk.org/jdk/pull/31123#discussion_r3460250059 PR Review Comment: https://git.openjdk.org/jdk/pull/31123#discussion_r3460856340 PR Review Comment: https://git.openjdk.org/jdk/pull/31123#discussion_r3460905186 PR Review Comment: https://git.openjdk.org/jdk/pull/31123#discussion_r3466436902 PR Review Comment: https://git.openjdk.org/jdk/pull/31123#discussion_r3466477300 PR Review Comment: https://git.openjdk.org/jdk/pull/31123#discussion_r3466497578 PR Review Comment: https://git.openjdk.org/jdk/pull/31123#discussion_r3466512171 PR Review Comment: https://git.openjdk.org/jdk/pull/31123#discussion_r3466547738 PR Review Comment: https://git.openjdk.org/jdk/pull/31123#discussion_r3466586329 PR Review Comment: https://git.openjdk.org/jdk/pull/31123#discussion_r3466631265
