On Thu, 24 Apr 2025 10:37:59 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:
>
> 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
