On Thu, 10 Apr 2025 14:19:25 GMT, Per Minborg <pminb...@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
