On Wed, 2 Apr 2025 13:22:44 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:
>
> Add info that Map#values and Map#entrySet are stable
Thanks for all the great work here, @minborg!
I've accumulated some comments, suggestions, and questions—I hope they are
helpful. Let me know if you want me to elaborate or explain anything.
(I haven't reviewed the benchmarks (yet))
src/java.base/share/classes/java/lang/StableValue.java line 271:
> 269: * public final class SqrtUtil {
> 270: *
> 271: * private SqrtUtil(){}
Suggestion:
* private SqrtUtil() {}
src/java.base/share/classes/java/lang/StableValue.java line 277:
> 275: * private static final Map<Integer, Double> SQRT =
> 276: * // @link substring="map" target="#map(Set,Function)" :
> 277: * StableValue.map(Set.of(1, 2, 4, 8, 16, 32),
> StrictMath::sqrt);
Why not used CACHED_KEYS here?
src/java.base/share/classes/java/lang/StableValue.java line 296:
> 294: *
> 295: * <h2 id="composition">Composing stable values</h2>
> 296: * A stable value can depend on other stable values, thereby creating a
> dependency graph
Suggestion:
* A stable value can depend on other stable values, forming a dependency graph
src/java.base/share/classes/java/lang/StableValue.java line 304:
> 302: * public final class DependencyUtil {
> 303: *
> 304: * private DependencyUtil(){}
Suggestion:
* private DependencyUtil() {}
src/java.base/share/classes/java/lang/StableValue.java line 353:
> 351: * }
> 352: *}
> 353: * Both {@code FIB} and {@code Fibonacci::fib} recurses into each other.
> Because the
Suggestion:
* Both {@code FIB} and {@code Fibonacci::fib} recurse into each other. Because
the
src/java.base/share/classes/java/lang/StableValue.java line 374:
> 372: * If there are circular dependencies in a dependency graph, a stable
> value will
> 373: * eventually throw a {@linkplain StackOverflowError} upon referencing
> elements in
> 374: * a circularity.
Is this still correct, I thought it detected reentrancy?
src/java.base/share/classes/java/lang/StableValue.java line 377:
> 375: *
> 376: * <h2 id="thread-safety">Thread Safety</h2>
> 377: * The content of a stable value is guaranteed to be set at most once.
> If competing
Suggestion:
* The contents of a stable value is guaranteed to be set at most once. If
competing
src/java.base/share/classes/java/lang/StableValue.java line 384:
> 382: * (e.g. {@linkplain #trySet(Object) trySet()})
> 383: * {@linkplain java.util.concurrent##MemoryVisibility
> <em>happens-before</em>}
> 384: * any subsequent read operation (e.g. {@linkplain #orElseThrow()}) that
> is successful.
Suggestion:
* any successful read operation (e.g. {@linkplain #orElseThrow()}).
src/java.base/share/classes/java/lang/StableValue.java line 426:
> 424: * {@linkplain java.lang.ref##reachability strongly
> reachable}. Clients are
> 425: * advised that {@linkplain java.lang.ref##reachability
> reachable} stable values
> 426: * will hold their set content perpetually.
This reads like the contents of StableValues will never be garbage collected,
even if the StableValue is no longer reachable. Is that intended?
src/java.base/share/classes/java/lang/StableValue.java line 567:
> 565: * <p>
> 566: * The provided {@code original} supplier is guaranteed to be
> successfully invoked
> 567: * at most once even in a multi-threaded environment. Competing
> threads invoking the
There's a bit of a mix of "at most once" and "at-most-once". I'm fine with
either but I prefer uniformity in spelling :)
src/java.base/share/classes/java/lang/StableValue.java line 589:
> 587:
> 588: /**
> 589: * {@return a new stable int function}
"int function" reads strange to me. If we look at something like IntStream, it
refers to "function":
https://docs.oracle.com/en/java/javase/24/docs/api/java.base/java/util/stream/IntStream.html#mapToObj(java.util.function.IntFunction)
src/java.base/share/classes/jdk/internal/lang/stable/StableEnumFunction.java
line 62:
> 60: @Override
> 61: public R apply(E value) {
> 62: if (!member.test(value.ordinal())) {
Suggestion:
if (!member.test(value.ordinal())) { // Implicit null-check of value
src/java.base/share/classes/jdk/internal/lang/stable/StableEnumFunction.java
line 89:
> 87: @Override
> 88: public String toString() {
> 89: final Collection<Map.Entry<E, StableValueImpl<R>>> entries = new
> ArrayList<>();
Might be good to size the entries to at most `size()`?
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();
I presume there's some external check to ensure that inputs is non-empty?
src/java.base/share/classes/jdk/internal/lang/stable/StableValueImpl.java line
93:
> 91: if (!trySet(content)) {
> 92: throw new IllegalStateException("Cannot set the content to "
> + content +
> 93: " because the content is already set: " +
> orElseThrow());
This might be problematic since it could surface confidential information
inside exception messages (or even just produce very large strings).
src/java.base/share/classes/jdk/internal/lang/stable/StableValueImpl.java line
125:
> 123: Objects.requireNonNull(supplier);
> 124: final Object t = wrappedContentAcquire();
> 125: return (t == null) ? orElseSetSlowPath(supplier) : unwrap(t);
Is it faster to have the slow path on the true-case?
src/java.base/share/classes/jdk/internal/lang/stable/StableValueImpl.java line
168:
> 166: private void preventReentry() {
> 167: if (Thread.holdsLock(this)) {
> 168: throw new IllegalStateException("Recursing supplier
> detected");
Perhaps "Recursive initialization is not supported"
src/java.base/share/classes/jdk/internal/lang/stable/StableValueImpl.java line
176:
> 174: assert Thread.holdsLock(this);
> 175: // This upholds the invariant, a `@Stable` field is written to
> at most once
> 176: // We know we hold the monitor here so plain semantic is enough
Perhaps better to move this to a javadoc/comment for the method itself: "must
only be invoked while holding the monitor"
src/java.base/share/classes/jdk/internal/lang/stable/StableValueImpl.java line
191:
> 189: @ForceInline
> 190: private static Object wrap(Object t) {
> 191: return (t == null) ? NULL_SENTINEL : t;
Personal preference, but it would seem more consistent to do the same check in
wrap as in unwrap:
Suggestion:
return (t != null) ? t : NULL_SENTINEL;
test/jdk/java/lang/StableValue/StableFunctionTest.java line 86:
> 84: basic(inputs, MAPPER);
> 85: basic(inputs, _ -> null);
> 86: }
Test for null mapper?
test/jdk/java/lang/StableValue/StableFunctionTest.java line 101:
> 99: assertTrue(cached.toString().endsWith("}"));
> 100: // One between the values
> 101: assertEquals(1L, cached.toString().chars().filter(ch -> ch ==
> ',').count(), cached.toString());
Perhaps factor out the testing of the individual methods to different test
cases to aid in debugging?
Perhaps only call `cached.toString()` once?
test/jdk/java/lang/StableValue/StableFunctionTest.java line 107:
> 105: assertTrue(x1.getMessage().contains("ILLEGAL"));
> 106: var x2 = assertThrows(IllegalArgumentException.class, () ->
> cached.apply(Value.ILLEGAL_AFTER));
> 107: assertTrue(x2.getMessage().contains("ILLEGAL"));
Only tests a part of the message?
test/jdk/java/lang/StableValue/StableFunctionTest.java line 115:
> 113: Function<Value, Integer> f0 = StableValue.function(inputs,
> Value::asInt);
> 114: Function<Value, Integer> f1 = StableValue.function(inputs,
> Value::asInt);
> 115: assertTrue(f0.toString().contains("{}"));
Not possible to test full string?
test/jdk/java/lang/StableValue/StableFunctionTest.java line 136:
> 134: assertTrue(cached.toString().contains(Value.THIRTEEN +
> "=.unset"));
> 135: assertTrue(cached.toString().contains(Value.FORTY_TWO +
> "=.unset"), cached.toString());
> 136: assertTrue(cached.toString().endsWith("}"));
Not possible to test full toString() representation?
test/jdk/java/lang/StableValue/StableFunctionTest.java line 146:
> 144: ref.set(cached);
> 145: cached.apply(Value.FORTY_TWO);
> 146: String toString = cached.toString();
I'd recommend caching the toString() representation in the other test cases as
well đź‘Ť
test/jdk/java/lang/StableValue/StableFunctionTest.java line 147:
> 145: cached.apply(Value.FORTY_TWO);
> 146: String toString = cached.toString();
> 147: assertTrue(toString.contains("(this StableFunction)"), toString);
Not possible to test full toString() representation?
test/jdk/java/lang/StableValue/StableListTest.java line 114:
> 112: Integer[] arr = new Integer[SIZE];
> 113: arr[INDEX] = 1;
> 114: assertSame(arr, StableValue.list(INDEX, IDENTITY).toArray(arr));
No test for contents?
test/jdk/java/lang/StableValue/StableListTest.java line 283:
> 281: var lazy = StableValue.list(SIZE, i -> ref.get().apply(i));
> 282: ref.set(lazy::get);
> 283: assertThrows(IllegalStateException.class, () -> lazy.get(INDEX));
Perhaps test the exception message to verify that it is indeed a recursion
detection?
test/jdk/java/lang/StableValue/StableMapTest.java line 143:
> 141: assertTrue(actual.contains(key + "=.unset"));
> 142: }
> 143: assertTrue(actual.endsWith("}"));
I guess the problem here is that the order of a map is not deterministic?
test/jdk/java/lang/StableValue/StableMapTest.java line 178:
> 176: }
> 177: assertTrue(entrySet.toString().startsWith("["));
> 178: assertTrue(entrySet.toString().endsWith("]"));
Cache toString() repr?
test/jdk/java/lang/StableValue/StableMapTest.java line 201:
> 199: } else {
> 200: assertTrue(map.toString().contains(key + "=.unset"));
> 201: }
Cache map.toString()
test/jdk/java/lang/StableValue/StableValueFactoriesTest.java line 36:
> 34: import static org.junit.jupiter.api.Assertions.*;
> 35:
> 36: final class StableValueFactoriesTest {
Just confirming—this is all that needs testing here?
test/jdk/java/lang/StableValue/StableValueTest.java line 262:
> 260: threads.forEach(StableValueTest::join);
> 261: // There can only be one winner
> 262: assertEquals(1, winner.cardinality());
AFAIK BitSet is not thread safe, so you have multiple threads writing to
it—which may or may not provide the right guarantees to lean on for this test.
Might be better to lean on CHM.
test/jdk/java/lang/StableValue/StableValuesSafePublicationTest.java line 64:
> 62: // fully initialized thanks to the HB properties of StableValue.
> 63: int a;
> 64: int b;
Might be worth putting in some more fields to increase the chance of detecting
a mismatch.
test/jdk/java/lang/StableValue/StableValuesSafePublicationTest.java line 153:
> 151: try {
> 152: for (Thread t:threads) {
> 153: long deadline =
> System.currentTimeMillis()+TimeUnit.MINUTES.toMillis(1);
Why currentTimeMillis() here and nanoTime() above?
test/jdk/java/lang/StableValue/TrustedFieldTypeTest.java line 120:
> 118: // assertThrows(IllegalAccessException.class, () -> {
> 119: field.set(stableValue, 13);
> 120: // });
Leftover comments?
-------------
PR Review: https://git.openjdk.org/jdk/pull/23972#pullrequestreview-2736446008
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2024928142
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2024929547
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2024930451
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2024930970
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2024932995
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2024942765
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2024943338
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2024946107
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2024954825
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2024978737
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2025012681
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2025045029
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2025073406
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2025077934
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2025200236
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2025201637
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2025364611
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2025365819
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2025368554
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2025377572
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2025380044
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2025380728
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2025381378
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2025383765
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2025384849
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2025384097
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2025388876
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2025392156
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2025395553
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2025396239
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2025396838
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2025400727
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2025404972
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2025407199
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2025409083
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2025411022
