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
Last review pass? :)
src/java.base/share/classes/java/lang/StableValue.java line 204:
> 202: * Set.of(1, 2, 4, 8, 16, 32);
> 203: * private static final UnaryOperator<Integer> UNDERLYING_LOG2 =
> 204: * i -> 31 - Integer.numberOfLeadingZeros(i);
Would boxing be necessary—i.e. what would be needed to use
java.util.function.IntUnaryOperator?
src/java.base/share/classes/java/lang/StableValue.java line 318:
> 316: * <p>
> 317: * Another example, which has a more complex dependency graph, is to
> lazily computing the
> 318: * Fibonacci sequence:
Suggestion:
* Another example, which has a more complex dependency graph, is to compute the
* Fibonacci sequence lazily:
src/java.base/share/classes/java/lang/StableValue.java line 410:
> 408: * parameter, and an {@linkplain #equals(Object) equals(obj)}
> parameter; all
> 409: * method parameters must be <em>non-null</em> or a {@link
> NullPointerException}
> 410: * will be thrown.
@minborg @mcimadamore I wonder if it makes more sense to specify that an
implementation is free to synchronize on `this`, and as such, as a user, it is
should be avoided to synchronize on StableValues.
This leaves the choice open to the implementations whether they want to
synchonize on `this` or use some other shceme, while still discouraging users
from synchronizing on StableValue instances.
src/java.base/share/classes/java/lang/StableValue.java line 415:
> 413: * a class and is usually neither exposed directly via
> accessors nor passed as
> 414: * a method parameter.
> 415: * Stable functions and collections make all reasonable
> efforts to provide
Suggestion:
* Stable functions and collections make reasonable efforts to provide
src/java.base/share/classes/java/lang/StableValue.java line 418:
> 416: * {@link Object#toString()} operations that do not trigger
> evaluation
> 417: * of the internal stable values when called.
> 418: * Stable collections have {@link Object#equals(Object)}
> operations that tries
Suggestion:
* Stable collections have {@link Object#equals(Object)} operations
that try
src/java.base/share/classes/java/lang/StableValue.java line 422:
> 420: * As objects can be set via stable values but never removed,
> this can be a source
> 421: * of unintended memory leaks. A stable value's content is
> 422: * {@linkplain java.lang.ref##reachability strongly
> reachable}. Clients are
I wonder if "Clients" is the best word here. Perhaps "Developers"?
src/java.base/share/classes/java/lang/StableValue.java line 487:
> 485: * <p>
> 486: * If the supplier throws an (unchecked) exception, the exception is
> rethrown, and no
> 487: * content is set. The most common usage is to construct a new
> object serving
Suggestion:
* If the supplier throws an (unchecked) exception, the exception is
rethrown and no
* content is set. The most common usage is to construct a new object
serving
src/java.base/share/classes/java/lang/StableValue.java line 571:
> 569: * thrown by the computing thread.
> 570: * <p>
> 571: * If the provided {@code underlying} supplier throws an exception,
> it is relayed
Suggestion:
* If the provided {@code underlying} supplier throws an exception, it is
rethrown
src/java.base/share/classes/java/lang/StableValue.java line 670:
> 668: * (e.g. via {@linkplain List#get(int) List::get}).
> 669: * <p>
> 670: * The provided {@code mapper} int function is guaranteed to be
> successfully invoked
Suggestion:
* The provided {@code mapper} function is guaranteed to be successfully
invoked
src/java.base/share/classes/java/lang/StableValue.java line 720:
> 718: * <p>
> 719: * If the provided {@code mapper} throws an exception, it is relayed
> to the initial
> 720: * caller and no value associated with the provided key is recorded.
Suggestion:
* If invoking the provided {@code mapper} function throws an exception, it
is rethrown to the initial
* caller and no value associated with the provided key is recorded.
src/java.base/share/classes/java/util/ImmutableCollections.java line 36:
> 34: import java.lang.reflect.Array;
> 35: import java.util.function.BiConsumer;
> 36: import java.util.function.BiFunction;
Make sure to update the copyright years of all the files modified in this PR.
test/jdk/java/lang/StableValue/StableMapTest.java line 140:
> 138: String actual = newMap().toString();
> 139: assertTrue(actual.startsWith("{"));
> 140: for (int key:KEYS) {
Suggestion:
for (int key : KEYS) {
test/jdk/java/lang/StableValue/StableMapTest.java line 317:
> 315: Consumer<Map<Integer, Integer>> consumer)
> implements Consumer<Map<Integer, Integer>> {
> 316: @java.lang.Override
> 317: public void accept(Map<Integer, Integer> map) {
> consumer.accept(map); }
Suggestion:
public void accept(Map<Integer, Integer> map) { consumer.accept(map); }
test/jdk/java/lang/StableValue/StableMapTest.java line 324:
> 322: static Stream<Operation> nullAverseOperations() {
> 323: return Stream.of(
> 324: new Operation("forEach", m -> m.forEach(null))
Suggestion:
new Operation("forEach", m -> m.forEach(null))
test/jdk/java/lang/StableValue/StableMapTest.java line 341:
> 339: new Operation("replace2", m -> m.replace(KEY,
> 1)),
> 340: new Operation("replace3", m -> m.replace(KEY,
> KEY, 1)),
> 341: new Operation("replaceAll", m -> m.replaceAll((a,
> _) -> a))
Suggestion:
new Operation("clear", Map::clear),
new Operation("compute", m -> m.compute(KEY, (_, _) ->
1)),
new Operation("computeIfAbsent", m -> m.computeIfAbsent(KEY, _ ->
1)),
new Operation("computeIfPresent", m -> m.computeIfPresent(KEY, (_,
_) -> 1)),
new Operation("merge", m -> m.merge(KEY, KEY, (a, _) ->
a)),
new Operation("put", m -> m.put(0, 0)),
new Operation("putAll", m -> m.putAll(Map.of())),
new Operation("remove1", m -> m.remove(KEY)),
new Operation("remove2", m -> m.remove(KEY, KEY)),
new Operation("replace2", m -> m.replace(KEY, 1)),
new Operation("replace3", m -> m.replace(KEY, KEY, 1)),
new Operation("replaceAll", m -> m.replaceAll((a, _) -> a))
-------------
PR Review: https://git.openjdk.org/jdk/pull/23972#pullrequestreview-2775117821
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2048580493
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2048568958
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2048697517
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2048698149
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2048698693
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2048700350
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2048703097
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2048705582
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2048707720
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2048709836
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2048720211
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2048722073
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2048723018
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2048723430
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2048724778
