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 I took a look at the java.lang.invoke changes + Unsafe. It mostly looks good to me, but I did find a few small issues. Left some comments inline. Just to recap my understanding here: - We currently do not support atomic access modes for fields and array elements whose flattened representation contains oops. This is expected to be temporary, so in the future all fields and array elements should go back to supporting atomic access modes. - Regardless of whether the VM chooses to flatten or not, we do not support atomic access modes for value types that have oops (hence the need for `VarHandleNonAtomicReferences`), in order to give users a consistent experience. (Again, temporary) - For arrays in particular, the layout of the array is polymorphic, so ArrayVarHandle needs to dynamically select the access method (reference v. flat), as well as other layout information. This is _not_ temporary. src/hotspot/share/opto/library_call.cpp line 3191: > 3189: > 3190: Node* map_addr = basic_plus_adr(mirror, field_map_offset); > 3191: const TypeAryPtr* val_type = > TypeAryPtr::INTS->cast_to_ptr_type(TypePtr::NotNull)->with_offset(0); Is the result of this load (the field map array) treated as stable? Maybe: Suggestion: const TypeAryPtr* val_type = TypeAryPtr::INTS->cast_to_ptr_type(TypePtr::NotNull)->with_offset(0)->cast_to_stable(true); I think this would allow folding subsequent loads from the array in `LoadNode::Value`. src/java.base/share/classes/java/lang/invoke/ArrayVarHandle.java line 2: > 1: /* > 2: * Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved. Might want to double check the copyright years. src/java.base/share/classes/java/lang/invoke/ArrayVarHandle.java line 147: > 145: int aoffset = (int) UNSAFE.arrayInstanceBaseOffset(array); > 146: int ascale = UNSAFE.arrayInstanceIndexScale(array); > 147: int ashift = 31 - Integer.numberOfLeadingZeros(ascale); Why not use `numberOfTrailingZeroes` here as well? (Just like for `REFERENCE_SHIFT`) Suggestion: int ashift = Integer.numberOfTrailingZeros(ascale); src/java.base/share/classes/java/lang/invoke/VarHandles.java line 152: > 150: : new > VarHandleReferences.FieldStaticReadWrite(decl, base, foffset, type, > f.isNullRestricted()); > 151: } else { > 152: return maybeAdapt(f.isFinal() && > !isWriteAllowedOnFinalFields Should the other branches use `maybeAdapt` as well? src/java.base/share/classes/jdk/internal/misc/Unsafe.java line 94: > 92: * {@code Unsafe} invocation, these assumptions may propagate backward to > 93: * previous statements, leading to wrong executions if the assumptions are > 94: * invalid. This is a great bit of new text that warns against incorrect use of Unsafe. But, I'll note that it leaves undefined what constitutes a correct use of Unsafe. For instance, this text says that invocations must be "conformant", but doesn't specify _to what_. It also mentions invocations with "illegal arguments", but doesn't specify how a user should determine if arguments are legal/illegal. I think the intent is for the specific restrictions to be described in more detail in the documentation of the individual methods. A reference to that in this text could be helpful I think. (not a blocker of course) src/java.base/share/classes/jdk/internal/misc/Unsafe.java line 358: > 356: * @return the value fetched from the indicated Java variable > 357: * @throws RuntimeException No defined exceptions are thrown, not > even > 358: * {@link NullPointerException} If this method doesn't throw any exceptions, I think the `@throws` should be removed. (same for the put) src/java.base/share/classes/jdk/internal/misc/Unsafe.java line 1730: > 1728: @ForceInline > 1729: public final <V> boolean compareAndSetReference(Object o, long > offset, > 1730: Class<?> type, `type` is unused here? Maybe this was intended to avoid the `getClass` in `isValueObject`? src/java.base/share/classes/jdk/internal/misc/Unsafe.java line 1765: > 1763: @ForceInline > 1764: public final <V> Object compareAndExchangeReference(Object o, long > offset, > 1765: Class<?> > valueType, Same here. `valueType` is unused. src/java.base/share/classes/jdk/internal/misc/Unsafe.java line 1881: > 1879: return compareAndSetReference(o, offset, valueType, > expected, x); > 1880: } else { > 1881: return weakCompareAndSetReferencePlain(o, offset, expected, > x); Looks like this is delegating to the wrong method Suggestion: return weakCompareAndSetReferenceAcquire(o, offset, expected, x); src/java.base/share/classes/jdk/internal/misc/Unsafe.java line 1908: > 1906: return compareAndSetReference(o, offset, valueType, > expected, x); > 1907: } else { > 1908: return weakCompareAndSetReferencePlain(o, offset, expected, > x); Same here Suggestion: return weakCompareAndSetReferenceRelease(o, offset, expected, x); src/java.base/share/classes/jdk/internal/misc/Unsafe.java line 1935: > 1933: return compareAndSetReference(o, offset, valueType, > expected, x); > 1934: } else { > 1935: return weakCompareAndSetReferencePlain(o, offset, expected, > x); Suggestion: return weakCompareAndSetReference(o, offset, expected, x); src/java.base/share/classes/jdk/internal/misc/Unsafe.java line 2929: > 2927: return false; > 2928: } > 2929: byte xByte = getByte(array, base + scale); // get x > as binary This can be outside the loop src/java.base/share/classes/jdk/internal/misc/Unsafe.java line 2933: > 2931: return true; > 2932: } > 2933: } while (true); I think there's a potential fast path here for values whose payloads don't have padding. This should always be the case for 1 and 2-byte payloads I think. In those cases, we could just do a binary compare and set. e.g. in this case: byte xByte = toByte(x); // notional function byte expectedByte = toByte(expected); return compareAndSetByte(o, offset, expectedByte, xByte); ------------- PR Review: https://git.openjdk.org/jdk/pull/31123#pullrequestreview-4526584104 PR Review Comment: https://git.openjdk.org/jdk/pull/31123#discussion_r3453526870 PR Review Comment: https://git.openjdk.org/jdk/pull/31123#discussion_r3452993511 PR Review Comment: https://git.openjdk.org/jdk/pull/31123#discussion_r3452584114 PR Review Comment: https://git.openjdk.org/jdk/pull/31123#discussion_r3453903962 PR Review Comment: https://git.openjdk.org/jdk/pull/31123#discussion_r3437224645 PR Review Comment: https://git.openjdk.org/jdk/pull/31123#discussion_r3452276388 PR Review Comment: https://git.openjdk.org/jdk/pull/31123#discussion_r3452759937 PR Review Comment: https://git.openjdk.org/jdk/pull/31123#discussion_r3452766550 PR Review Comment: https://git.openjdk.org/jdk/pull/31123#discussion_r3452409557 PR Review Comment: https://git.openjdk.org/jdk/pull/31123#discussion_r3452411459 PR Review Comment: https://git.openjdk.org/jdk/pull/31123#discussion_r3452416829 PR Review Comment: https://git.openjdk.org/jdk/pull/31123#discussion_r3452652493 PR Review Comment: https://git.openjdk.org/jdk/pull/31123#discussion_r3452916171
