On Tue, 1 Apr 2025 13:27:34 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 two additional
> commits since the last revision:
>
> - Add lazy toSting for StableMap::values
> - Make toString for reversed and sublist lazy
src/java.base/share/classes/java/lang/StableValue.java line 257:
> 255: * which are held by stable values:
> 256: * {@snippet lang = java:
> 257: * public final class DependencyUtil {
I think the fibonacci example is great -- and is probably enough for this
section. We also had, at some point, a nice ASCII art of the computations
involved in the fibonacci, which might be useful to illustrate your claim re.
"dependency graph". Do you think you can recover that ?
src/java.base/share/classes/java/lang/StableValue.java line 327:
> 325: * (e.g. {@linkplain #trySet(Object) trySet()})
> 326: * {@linkplain java.util.concurrent##MemoryVisibility
> <em>happens-before</em>}
> 327: * any subsequent read operation (e.g. {@linkplain #orElseThrow()}).
Maybe we should clarify that a write of V happens before a read that can see
that can see that V. E.g. a successful `trySet` doesn't happen before _any_
`orElseThrow` _in general_ -- (because `orElseThrow` can also throw -- in which
case you know you arrived too early). Also, there's operations in there like
`orElseSet` which both get and set -- so perhaps things should be specified a
bit more -- e.g. maybe we should talk about a successful read operation as:
* `orElseThrow` that doesn't throw
* `orElse` which dosn't return the fallback value
* `orElseGet` that doesn't run the provided supplier
We can also talk about a succseful write operation:
* a `trySet` which returns true
* a `setOrThrow` that doesn't throw
* an `orElseSet` that runs the supplier
Then we can say that a _successful_ write operation happens before a
_successful_ read operation.
src/java.base/share/classes/java/lang/StableValue.java line 371:
> 369:
> 370: /**
> 371: * {@return {@code true} if the content was set to the provided
> {@code value},
I find this description a bit confusing -- as written it almost looks like a
predicate -- e.g. test if this stable value is set with the value I'm
providing. But the important thing here is the side-effect -- so I feel the
javadoc for this method should be much less about what this method returns, and
much more about what the method actually does. (`setOrThrow` seems to be better
in this respect)
src/java.base/share/classes/java/lang/StableValue.java line 378:
> 376: * @param value to set
> 377: */
> 378: boolean trySet(T value);
This can probably throw `IllegalStateException` too right -- e.g. if used
inside a supplier passed to `orElseSet` ?
src/java.base/share/classes/java/lang/StableValue.java line 416:
> 414: * When this method returns successfully, the content is always set.
> 415: * <p>
> 416: * This method will always return the same witness value regardless
> if invoked by
will always return/always returns (more direct)
src/java.base/share/classes/java/lang/StableValue.java line 435:
> 433: *
> 434: * @param value to set
> 435: * @throws IllegalStateException if the content was already set
The javadoc speaks about "a stable value is set", and not "content is set".
Make sure we're using terminology consistenty
src/java.base/share/classes/java/lang/StableValue.java line 461:
> 459: * An unset stable value has no content.
> 460: * <p>
> 461: * The returned stable value is not {@link Serializable}.
I find the claim about serializable a bit odd here. Perhaps in some
`implSpec`/`apiNote` at the toplevel?
src/java.base/share/classes/java/lang/StableValue.java line 507:
> 505: * @param <T> the type of results supplied by the returned
> supplier
> 506: */
> 507: static <T> Supplier<T> supplier(Supplier<? extends T> original) {
I think that here and in all other functional/collection factories, we should
document that `toString` will never cause a value to be computed.
src/java.base/share/classes/java/lang/StableValue.java line 591:
> 589: * <p>
> 590: * The returned list is an {@linkplain Collection##unmodifiable
> unmodifiable} list
> 591: * whose size is known at construction. The list's elements are
> computed via the
s/"is known at construction"/`{@code size}` ?
src/java.base/share/classes/java/lang/StableValue.java line 603:
> 601: * caller and no value for the element is recorded.
> 602: * <p>
> 603: * The returned list and its {@link List#subList(int, int) subList}
> views implement
Perhaps the most important fact to note here is that the returned lists via
`subList` and friends are _also_ stable lists!
src/java.base/share/classes/java/lang/StableValue.java line 657:
> 655: * @param <V> the type of mapped values in the returned map
> 656: */
> 657: static <K, V> Map<K, V> map(Set<K> keys,
Like `List`, a map can also generate other kinds of collections:
1. an entry set (`Map::entrySet`)
2. a key set (`Map::keySet`)
3. a value set (`Map::values`)
We should make it clear that calling (1) and (3) will result in materializing
the values for the map -- we do not have "lazy" views for such sets.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2024400960
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2024413931
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2024425037
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2024438670
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2024431337
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2024442624
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2024445136
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2024469913
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2024451560
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2024454122
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2024461588
