Il Lun 22 Giu 2026, 19:47 <[email protected]> ha scritto:
> Send net-dev mailing list submissions to > [email protected] > > To subscribe or unsubscribe via email, send a message with subject or > body 'help' to > [email protected] > > You can reach the person managing the list at > [email protected] > > When replying, please edit your Subject line so it is more specific > than "Re: Contents of net-dev digest..." > > Today's Topics: > > 1. Re: RFR: 8317279: Standard library implementation of value classes > and objects [v5] > (Jorn Vernee) > 2. Re: RFR: 8317279: Standard library implementation of value classes > and objects [v5] > (Jorn Vernee) > > > ---------------------------------------------------------------------- > > Message: 1 > Date: Mon, 22 Jun 2026 17:45:12 GMT > From: Jorn Vernee <[email protected]> > Subject: Re: RFR: 8317279: Standard library implementation of value > classes and objects [v5] > To: <[email protected]>, <[email protected]>, > <[email protected]>, <[email protected]>, > <[email protected]>, <[email protected]>, > <[email protected]>, <[email protected]>, > <[email protected]>, <[email protected]> > Message-ID: <gSDOIodFjzyTE8Ls3CwAe4hahqgJfNG-Fs8YvZuHMxw=.989f2648-2c > [email protected]> > Content-Type: text/plain; charset=utf-8 > > 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 > > ------------------------------ > > Message: 2 > Date: Mon, 22 Jun 2026 17:45:13 GMT > From: Jorn Vernee <[email protected]> > Subject: Re: RFR: 8317279: Standard library implementation of value > classes and objects [v5] > To: <[email protected]>, <[email protected]>, > <[email protected]>, <[email protected]>, > <[email protected]>, <[email protected]>, > <[email protected]>, <[email protected]>, > <[email protected]>, <[email protected]> > Message-ID: <C8PcYPVhrzaXlF5W5puSL1ylsJwn2aX8rRrOaOmu_3U=.cf55552e-36 > [email protected]> > Content-Type: text/plain; charset=utf-8 > > On Mon, 22 Jun 2026 13:42:03 GMT, Jorn Vernee <[email protected]> wrote: > > >> 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 > > > > 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 > > I think that also means you could re-use a single array element for both > values. > > ------------- > > PR Review Comment: > https://git.openjdk.org/jdk/pull/31123#discussion_r3452676586 > > End of net-dev Digest, Vol 228, Issue 62 > **************************************** >
