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

Jorn Vernee jvernee at openjdk.org
Fri Apr 18 15:20:16 UTC 2025 

On Thu, 10 Apr 2025 14:19:25 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:
> 
>   Address comments on original vs underlying

Did an initial pass over just the implementation. Will look at the tests later.

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

> 1: /*
> 2:  * Copyright (c) 2024, 2025, Oracle and/or its affiliates. All rights reserved.

As far as I know, when the files are integrated into the mainline JDK, the copyright year should be set to the current year. For the FFM API we also had to reset all the years, even though the files had existed for a few years in the panama-foreign repo.

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

> 116:  * The {@code getLogger()} method calls {@code logger.orElseSet()} on the stable value to
> 117:  * retrieve its content. If the stable value is <em>unset</em>, then {@code orElseSet()}
> 118:  * evaluates and sets the content; the content is then returned to the client. In other

What does 'orElseSet' evaluate? Feels like there's a bit missing in this sentence. Maybe:

Suggestion:

 * retrieve its content. If the stable value is <em>unset</em>, then {@code orElseSet()}
 * evaluates the given supplier, and sets the content to the result; the content is then returned to the client. In other

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

> 124:  * This property is crucial as evaluation of the supplier may have side effects,
> 125:  * e.g., the call above to {@code Logger.create()} may result in storage resources
> 126:  * being prepared.

I think it's better to avoid abbreviations like 'e.g.' in javadoc, and go for the full 'for example'.

Also, WRT the contents of this paragraph: if multiple threads execute the `orElseSet` statement concurrently, multiple different `Supplier` instances might be created and passed to `orElseSet`. I think the method guarantees that only one of these (multiple) suppliers will be evaluated. But, technically, there is no 'the supplier', I suppose.

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

> 280:  * A stable value can depend on other stable values, forming a dependency graph
> 281:  * that can be lazily computed but where access to individual elements still can be
> 282:  * performant. In the following example, a single {@code Foo} and a {@code Bar}

Word order doesn't seem right here.

Suggestion:

 * that can be lazily computed but where access to individual elements can still be
 * performant. In the following example, a single {@code Foo} and a {@code Bar}

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

> 342:  * <p>
> 343:  * The fibonacci example above is a dependency graph with no circular dependencies (i.e.,
> 344:  * it is a dependency tree):

I believe the technical term here is 'directed acyclic graph'

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

> 361:  * The content of a stable value is guaranteed to be set at most once. If competing
> 362:  * threads are racing to set a stable value, only one update succeeds, while other updates
> 363:  * are blocked until the stable value becomes set.

I think it would be good to say here that the blocked updates will be discarded, or something to that effect. The current sentence can be interpreted as the blocked updates still taking place after the SV becomes set.

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

> 383:  * The method {@link #orElseSet(Supplier)} guarantees that the provided
> 384:  * {@linkplain Supplier} is invoked successfully at most once even under race.
> 385:  * Invocations of {@link #setOrThrow(Object)} forms a total order of zero or more

Suggestion:

 * Invocations of {@link #setOrThrow(Object)} form a total order of zero or more

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

> 387:  * successful invocation. Since stable functions and stable collections are built on top
> 388:  * of {@linkplain StableValue#orElseSet(Supplier) orElseSet()} they too are
> 389:  * thread safe and guarantee at-most-once-per-input invocation.

I think you should drop the note here about stable collections being built on top of `orElseSet`. This is an interesting implementation detail, but not directly relevant for the specification, and some that we might want to change in the future.

I suggest just specifying the behavior of stable collections directly, without mentioning how they are implemented.

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

> 394:  * stable value itself is stored in a {@code static final} field). Stable functions and
> 395:  * collections are built on top of StableValue. As such, they are also treated as
> 396:  * constants by the JVM.

This doesn't seem quite correct. stable collections aren't necessarily treated as constant, you need a constant reference, e.g. stored in a static final field. The contents can be treated as constant though.

Suggestion:

 * collections are built on top of StableValue. As such, their contents is also treated as
 * constant by the JVM.

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

> 398:  * This means that, at least in some cases, access to the content of a stable value
> 399:  * enjoys the same constant-folding optimizations that are available when accessing
> 400:  * {@code static final} fields.

This text seems too normative to me. Whether a JVM implementation chooses to constant fold things is up to that JVM implementation, and not something that is required of _all_ JVM implementations.

I think you could say something like: "since the contents of a stable value can never change after it has been set, a JVM implementation may, for a set stable value, elide all future reads of that stable value, and instead directly use any contents that it has previously observed"

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

> 403:  *           {@code this} and consequently, care should be taken whenever
> 404:  *           (directly or indirectly) synchronizing on a {@code StableValue}. Failure to
> 405:  *           do this may lead to deadlock. Stable functions and collections on the

Given the previous sentence, in this sentence, 'this' could interpreted as 'synchronizing on a stable value' (and failing to do so may lead to dealock). You might want to reword this a bit.

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

> 430:  *           stable values of arbitrary depth and still provide transitive constantness.
> 431:  *           Stable values, functions and collections are not {@link Serializable}.
> 432:  *

This last sentence should probably be a separate implSpec

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

> 452:      * @param content to set
> 453:      * @throws IllegalStateException if this method is invoked directly by a supplier
> 454:      *         provided to the {@link #orElseSet(Supplier)} method.

Why does this result in an exception? Given the example of dependent SV's I thought it would be fine or e.g. a stable value A to initialize another stable value B using `trySet`.

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

> 567:      * returned supplier's {@linkplain Supplier#get() get()} method when a value is
> 568:      * already under computation will block until a value is computed or an exception is
> 569:      * thrown by the computing thread.

Similar here, I think you could add a note saying that blocked threads will not evaluate their supplier after resuming if the SV has already been set.

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

> 590:      * function upon being first accessed via the returned function's
> 591:      * {@linkplain IntFunction#apply(int) apply()} method. If the returned function is
> 592:      * invoked with an input that is not allowed, an {@link IllegalArgumentException}

Same here:

Suggestion:

     * invoked with an input that is not in the range {@code [0, size)}, an {@link IllegalArgumentException}

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

> 604:      * <p>
> 605:      * If the provided {@code underlying} function recursively calls the returned
> 606:      * function for the same index, an {@linkplain IllegalStateException} will

This is the only place where I see the word 'index' used. Elsewhere you use 'input'. Might want to switch to the latter here as well.

Suggestion:

     * function for the same input, an {@linkplain IllegalStateException} will

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

> 607:      * be thrown.
> 608:      *
> 609:      * @param size       the size of the allowed inputs in {@code [0, size)}

I think it's worth mentioning somewhere that the inputs to the int function are contiguous, starting at `0`.

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

> 615:                                           IntFunction<? extends R> underlying) {
> 616:         if (size < 0) {
> 617:             throw new IllegalArgumentException();

Please add an exception message.

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

> 628:      * {@code underlying} function upon being first accessed via the returned function's
> 629:      * {@linkplain Function#apply(Object) apply()} method. If the returned function is
> 630:      * invoked with an input that is not allowed, an {@link IllegalArgumentException}

I think 'not allowed' is too vague.

Suggestion:

     * invoked with an input that is not in {@code inputs}, an {@link IllegalArgumentException}

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

> 645:      *
> 646:      * @param inputs     the set of (non-null) allowed input values
> 647:      * @param underlying Function used to compute cached values

Looks like some code tags are missing here and for `intFunction`?

Suggestion:

     * @param underlying {@code Function} used to compute cached values

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

> 649:      * @param <R>        the type of results delivered by the returned Function
> 650:      * @throws NullPointerException if the provided set of {@code inputs} contains a
> 651:      *                              {@code null} element.

This doesn't seem to be checked by this method.

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

> 698:                             IntFunction<? extends E> mapper) {
> 699:         if (size < 0) {
> 700:             throw new IllegalArgumentException();

Please add an exception message here too.

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

> 736:      * @param <V>    the type of mapped values in the returned map
> 737:      * @throws NullPointerException if the provided set of {@code inputs} contains a
> 738:      *                              {@code null} element.

Here also, this does not seem to be checked by this method.

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

> 1590:                     final K k = inner.getKey();
> 1591:                     return new NullableKeyValueHolder<>(k, inner.getValue().orElseSet(new Supplier<V>() {
> 1592:                         @Override public V get() { return mapper.apply(k); }}));

I suppose you could potentially make this more lazy by returning an `Entry` implementation that only evaluates the mapper when calling `Entry::getValue`.

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

> 1628:                         @SuppressWarnings("unchecked")
> 1629:                         var array = (StableValueImpl<V>[]) Array.newInstance(StableValueImpl.class, len);
> 1630:                         return array;

No need to use reflection here, since the array type is statically known.

Suggestion:

                        return new StableValueImpl[len];

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

> 69:         } catch (ArrayIndexOutOfBoundsException ioob) {
> 70:             throw new IllegalArgumentException("Input not allowed: " + value, ioob);
> 71:         }

This AIOOBE should not be possible, right? The `member` test already checks that the key is valid. Meaning that, if an AIOOBE is thrown here, there's a bug elsewhere.

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

> 69:         } catch (ArrayIndexOutOfBoundsException ioob) {
> 70:             throw new IllegalArgumentException("Input not allowed: " + value, ioob);
> 71:         }

Suggestion:

        final StableValueImpl<R> delegate = delegates[index];

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

> 109:             min = Math.min(min, ordinal);
> 110:             max = Math.max(max, ordinal);
> 111:             bitSet.set(ordinal);

This doesn't look right wrt the way the bitset is initialized. `ordinal` can be larger than `inputs.size()`. For example for `enum X { A, B, C }` where the key set is `{ C }`.

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

> 113: 
> 114:         final int size = max - min + 1;
> 115:         final Class<E> enumType = (Class<E>)inputs.iterator().next().getClass();

Maybe you could make the type of `input` EnumSet, and then grab the element type directly.

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

> 25:         boolean first = true;
> 26:         for (int i = 0; i < length; i++) {
> 27:             if (first) { first = false; } else { sb.append(", "); }

This kind of behavior can be achieved using `StringJoiner` instead of `StringBuilder`.

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

> 61:     public static <T> StableValueImpl<T>[] array(int size) {
> 62:         if (size < 0) {
> 63:             throw new IllegalArgumentException();

Please add an exception message

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

> 94:     public void setOrThrow(T content) {
> 95:         if (!trySet(content)) {
> 96:             // Neither the set content nor the provided content is reveled in the

Suggestion:

            // Neither the set content nor the provided content is revealed in the

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

> 171:     private void preventReentry() {
> 172:         if (Thread.holdsLock(this)) {
> 173:             throw new IllegalStateException("Recursive initialization is not supported");

Prevent users from having to look through the stack trace. Also, 'supported' sounds like it might be implemented later. I think 'allowed' is better to signal that this is an invariant.

Suggestion:

            throw new IllegalStateException("Recursive initialization of a StableValue is not allowed");

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

PR Review: https://git.openjdk.org/jdk/pull/23972#pullrequestreview-2776306916
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2050719259
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2049279945
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2049293638
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2050544213
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2050547076
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2050562079
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2050578035
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2050583697
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2050589462
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2050594539
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2050604082
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2050612306
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2050617637
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2050631923
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2050647058
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2050647863
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2050637730
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2050640199
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2050643027
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2050651143
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2050652258
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2050654014
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2050691198
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2050713377
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2050716955
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2050734308
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2050735452
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2050727569
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2050732724
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2050740555
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2050743057
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2050746822
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2050751830

More information about the core-libs-dev mailing list