RFR: 8351565: Implement JEP 502: Stable Values (Preview) [v78]

Luca Kellermann duke at openjdk.org
Fri Apr 25 03:45:10 UTC 2025


On Thu, 24 Apr 2025 10:37:59 GMT, Per Minborg <pminborg at openjdk.org> wrote:

>> Implement JEP 502.
>> 
>> The PR passes tier1-tier3 tests.
>
> Per Minborg has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Make public constuctor private

src/java.base/share/classes/java/lang/StableValue.java line 389:

> 387:  * Invocations of {@link #setOrThrow(Object)} form a total order of zero or more
> 388:  * exceptional invocations followed by zero (if the contents were already set) or one
> 389:  * successful invocation. Since stable functions and stable collections are built on top

How can an exceptional invocation of `setOrThrow` be followed by a successful invocation?

src/java.base/share/classes/java/lang/StableValue.java line 447:

> 445:         permits StableValueImpl {
> 446: 
> 447:     // Principal methods

Not important, not practical and hacky (exceptions as control flow), but technically all methods could be implemented in terms of `orElseSet`, so they could also be called "convenience methods":


trySet(c) {
    success = false
    orElseSet(() -> {success = true; return c})
    return succes
}

orElse(o) {
    try { return orElseSet(() -> throw) }
    catch { return o }
}

orElseThrow() {
    orElseSet(() -> throw)
}

isSet() {
    try { orElseSet(() -> throw) }
    catch { return false }
    return true
}

setOrThrow(c) {
    success = false
    orElseSet(() -> {success = true; return c})
    if (!success) throw
}

I guess these two comments (`// Principal methods` and `// Convenience methods`) could also just be omitted.

src/java.base/share/classes/java/lang/StableValue.java line 523:

> 521:      * @throws IllegalStateException if the contents was already set
> 522:      */
> 523:     void setOrThrow(T contents);

Should probably also mention that `IllegalStateException` can be caused by recursive initialization (see `trySet`).

src/java.base/share/classes/java/lang/StableValue.java line 577:

> 575:      * already under computation will block until a value is computed or an exception is
> 576:      * thrown by the computing thread. The computing threads will then observe the newly
> 577:      * computed value (if any) and will then never execute.

This sentence seems off. I think it should say "competing threads" instead of "computing threads". And what will they never execute?

src/java.base/share/classes/java/lang/StableValue.java line 688:

> 686:      * <p>
> 687:      * Any direct {@link List#subList(int, int) subList} or {@link List#reversed()} views
> 688:      * of the returned list are also stable.

What about non-direct views? I think they could be made stable (i.e. have a non-evaluating `toString`) with few changes:

Override `subList` in `ImmutableCollections.StableList.StableReverseOrderListView` similar to `ReverseOrderListView.subList`:

@Override
public List<E> subList(int fromIndex, int toIndex) {
    int size = base.size();
    subListRangeCheck(fromIndex, toIndex, size);
    return new StableReverseOrderListView<>(base.subList(size - toIndex, size - fromIndex));
}


Override `reversed` in `ImmutableCollections.SubList`:

@Override
public List<E> reversed() {
    if (root instanceof StableList) {
        return new StableList.StableReverseOrderListView<>(this);
    } else {
        return super.reversed();
    }
}


Change `ImmutableCollections.StableList.StableReverseOrderListView.toString` (instead of copying and reversing, `StableUtil.renderElements` could also be modified to have a configurable iteration order):

@Override
public String toString() {
    final StableList<E> stable;
    final int from, to;
    if (base instanceof SubList<E> sub) {
        stable = (StableList<E>)sub.root;
        from = sub.offset;
        to = sub.offset + sub.size();
    } else {
        stable = (StableList<E>)base;
        from = 0;
        to = stable.delegates.length;
    }
    final StableValueImpl<E>[] reversed = ArraysSupport.reverse(
            Arrays.copyOfRange(stable.delegates, from, to));
    return StableUtil.renderElements(this, "StableList", reversed);
}


That should make sure that however many times and in whatever combination `reversed` and `subList` are called on a stable list, it will always be an instance of `ImmutableCollections.StableList`, `ImmutableCollections.StableList.StableReverseOrderListView` or `ImmutableCollections.SubList`. They all properly override `toString`. The view nesting depth is also bounded.

Visualized as a table (`SL` = `ImmutableCollections.StableList`, `Rev` = `ImmutableCollections.StableList.StableReverseOrderListView`, `Sub` = `ImmutableCollections.SubList`):
| view nesting  | calling `reversed` on that view returns | calling `subList` on that view returns |
| ------------- | --------------------------------------- | -------------------------------------- |
| `SL`          | `Rev(SL)`                               | `Sub(SL)`                              |
| `Rev(SL)`     | `SL`                                    | `Rev(Sub(SL))`                         |
| `Sub(SL)`     | `Rev(Sub(SL)`                           | `Sub(SL)`                              |
| `Rev(Sub(SL)` | `Sub(SL)`                               | `Rev(Sub(SL))`                         |

src/java.base/share/classes/java/lang/StableValue.java line 731:

> 729:      * <p>
> 730:      * Any direct {@link Map#values()} or {@link Map#entrySet()} views
> 731:      * of the returned map are also stable.

"direct views" implies there are non-direct views. What would these be? `Collection` and `Set` have no methods that return views. The only non-direct view I can think of are the `Map.Entry` instances in the entry set, but see https://github.com/openjdk/jdk/pull/23972#discussion_r2050713377.

src/java.base/share/classes/java/util/ImmutableCollections.java line 270:

> 268:         // A lazy list is not Serializable so, we cannot return `List.of()` if size == 0
> 269:         return new StableList<>(size, mapper);
> 270:     }

Why have this function for stable lists but not for maps? Also uses "lazy list" instead of "stable list" in the comment.

src/java.base/share/classes/java/util/ImmutableCollections.java line 578:

> 576:         public String toString() {
> 577:             if (root instanceof StableList<E> stableList) {
> 578:                 return StableUtil.renderElements(root, "StableList", stableList.delegates, offset, size);

Suggestion:

                return StableUtil.renderElements(this, "StableList", stableList.delegates, offset, size);

Other view collections (`ArrayList.SubList`, `ReverseOrderListView`, `AbstractMap.keySet`, `AbstractMap.values`, `HashMap.EntrySet`) use the view reference (and not the underlying collection) for detecting self containment, so `renderElements` should use `this` instead of `root` for `self`.

src/java.base/share/classes/java/util/ImmutableCollections.java line 898:

> 896:                 final StableValueImpl<E>[] reversed = ArraysSupport.reverse(
> 897:                         Arrays.copyOf(delegates, delegates.length));
> 898:                 return StableUtil.renderElements(base, "Collection", reversed);

Suggestion:

                return StableUtil.renderElements(this, "StableList", reversed);

All other calls use "Stable...". Other view collections (`ArrayList.SubList`, `ReverseOrderListView`, `AbstractMap.keySet`, `AbstractMap.values`, `HashMap.EntrySet`) use the view reference (and not the underlying collection) for detecting self containment, so `renderElements` should use `this` instead of `base` for `self`.

src/java.base/share/classes/java/util/ImmutableCollections.java line 1660:

> 1658:             public String toString() {
> 1659:                 final StableValueImpl<?>[] values = delegate.values().toArray(GENERATOR);
> 1660:                 return StableUtil.renderElements(StableMap.this, "StableMap", values);

Suggestion:

                return StableUtil.renderElements(this, "StableCollection", values);

Other view collections (`ArrayList.SubList`, `ReverseOrderListView`, `AbstractMap.keySet`, `AbstractMap.values`, `HashMap.EntrySet`) use the view reference (and not the underlying collection) for detecting self containment, so `renderElements` should use `this` instead of `StableMap.this` for `self`. `ImmutableCollections.StableMap.StableMapValues` is a `Collection`, not a `Map`, so use `"StableCollection"` instead of `"StableMap"`.

src/java.base/share/classes/jdk/internal/lang/stable/StableEnumFunction.java line 51:

> 49:  * @param firstOrdinal the lowest ordinal used
> 50:  * @param delegates    a delegate array of inputs to StableValue mappings
> 51:  * @param original     the original Function

Are `enumType` and `member` missing intentionally?

src/java.base/share/classes/jdk/internal/lang/stable/StableEnumFunction.java line 70:

> 68:         // Since we did the member.test above, we know the index is in bounds
> 69:         delegate = delegates[index];
> 70:         return delegate.orElseSet(new Supplier<R>() {

Suggestion:

        // Since we did the member.test above, we know the index is in bounds
        return delegates[index].orElseSet(new Supplier<R>() {

I don't think the local variable is needed here.

src/java.base/share/classes/jdk/internal/lang/stable/StableEnumFunction.java line 88:

> 86:     public String toString() {
> 87:         final E[] enumElements = enumType.getEnumConstants();
> 88:         final Collection<Map.Entry<E, StableValueImpl<R>>> entries = new ArrayList<>(enumElements.length);

Suggestion:

        final Collection<Map.Entry<E, StableValueImpl<R>>> entries = new ArrayList<>(delegates.length);

Otherwise the list is bigger than needed (even `delegates.length` could be too big but should always be smaller than `enumElements.length`).

src/java.base/share/classes/jdk/internal/lang/stable/StableEnumFunction.java line 102:

> 100:                                                               Function<? super T, ? extends R> original) {
> 101:         // The input set is not empty
> 102:         final Class<E> enumType = (Class<E>)inputs.iterator().next().getClass();

Suggestion:

        final Class<E> enumType = ((E)inputs.iterator().next()).getDeclaringClass();

`Class.getEnumConstants` returns `null` for the class object of an enum constant with a body. This would lead to a NPE in the next line.

This code should trigger it:

enum E {
    A, B { public String toString() { return "b"; } }
}

StableValue.function(EnumSet.of(E.B), x -> x);

src/java.base/share/classes/jdk/internal/lang/stable/StableFunction.java line 35:

> 33: import java.util.function.Supplier;
> 34: 
> 35: // Note: It would be possible to just use `LazyMap::get` with some additional logic

Is there a class named `LazyMap`? Or was this comment not updated to use the "stable" term?

src/java.base/share/classes/jdk/internal/lang/stable/StableIntFunction.java line 36:

> 34: // Note: It would be possible to just use `LazyList::get` instead of this
> 35: // class but explicitly providing a class like this provides better
> 36: // debug capability, exception handling, and may provide better performance.

This comment seems somewhat redundant, at least performance is already mentioned in the javadoc comment right beneath it. Also `LazyList`, see my comment for `StableFunction`.

src/java.base/share/classes/jdk/internal/lang/stable/StableUtil.java line 69:

> 67:             final String valueString;
> 68:             if (value == self) {
> 69:                 valueString = ("(this ") + selfName + ")";

Suggestion:

                valueString = "(this " + selfName + ")";

src/java.base/share/classes/jdk/internal/lang/stable/StableValueImpl.java line 57:

> 55:             UNSAFE.objectFieldOffset(StableValueImpl.class, "contents");
> 56:     // Used to indicate a holder value is `null` (see field `value` below)
> 57:     // A wrapper method `nullSentinel()` is used for generic type conversion.

Suggestion:

    // Used to indicate a holder value is `null` (see field `contents` below)

The `nullSentinel` method no longer exists, the field is named `contents`.

src/java.base/share/classes/jdk/internal/lang/stable/StableValueImpl.java line 86:

> 84:         preventReentry();
> 85:         // Mutual exclusion is required here as `orElseSet` might also
> 86:         // attempt to modify the `wrappedValue`

Suggestion:

        // attempt to modify `this.contents`

src/java.base/share/classes/jdk/internal/lang/stable/StableValueImpl.java line 161:

> 159: 
> 160:     @ForceInline
> 161:     public Object wrappedContentAcquire() {

Maybe name it `wrappedContentsAcquire` (with an "s")? It's called "contents" elsewhere.

src/java.base/share/classes/jdk/internal/lang/stable/StableValueImpl.java line 188:

> 186:      */
> 187:     @ForceInline
> 188:     private boolean wrapAndSet(Object newValue) {

Suggestion:

    private boolean wrapAndSet(T newValue) {

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2058468433
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2058510873
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2058567306
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2058875533
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2058903119
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2058900859
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2059151416
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2059388757
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2059383526
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2059476366
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2058957558
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2058968283
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2058974011
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2059008635
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2058940541
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2058945089
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2059387502
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2058555891
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2058574430
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2058598520
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2058662923


More information about the core-libs-dev mailing list