On Wed, 12 Nov 2025 10:07:39 GMT, Per Minborg <[email protected]> wrote:
>> Implement JEP 526: Lazy Constants (Second Preview) >> >> The lazy list/map implementations are broken out from `ImmutableCollections` >> to a separate class. >> >> The old benchmarks are not moved/renamed to allow comparison with previous >> releases. >> >> `java.util.Optional` is updated so that its field is annotated with >> `@Stable`. This is to allow `Optional` instances to be held in lazy >> constants and still provide constant folding. > > Per Minborg has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 112 commits: > > - Clarify toString spec > - Merge branch 'master' into lazy-constants > - Add @AOTSafeClassInitializer > - Address comments in PR > - Fix merge mistake > - Merge master > - Rework toString implementations > - Update after doc comments > - Merge branch 'master' into lazy-constants > - Revert the AbstractMap.keySet @Stable annotation > - ... and 102 more: https://git.openjdk.org/jdk/compare/76a1109d...1f439bec Some initial comments on the spec and implementation. I still need to look at the tests. src/java.base/share/classes/java/lang/LazyConstant.java line 48: > 46: * (provided at construction) will be invoked and the result will be used > to initialize > 47: * the constant. Once a lazy constant is initialized, its contents can > <em>never change</em> > 48: * and will be retrieved over and over again upon subsequent {@linkplain > #get() get} Are the above links to `#get()` also intended to have a the plain `get` text? src/java.base/share/classes/java/lang/LazyConstant.java line 169: > 167: * a lazy constant {@linkplain java.lang.ref##reachability > strongly references} > 168: * it contents. Hence, a lazy constant will hold its contents > until > 169: * the lazy constant itself is collected (if ever). Maybe you could avoid the word 'collected' here, as there are no other references to the GC in this section, and no link to what GC is/does. i.e. I think 'collected' is not well-defined enough to be used here. Suggestion: * it contents. Hence, the contents of a lazy constant will be reachable as long * as the lazy constant itself is reachable. src/java.base/share/classes/java/lang/LazyConstant.java line 249: > 247: > 248: /** > 249: * {@return the {@linkplain System#identityHashCode(Object)} for > this lazy constant} How does this link render? It doesn't have any plain text. Maybe it's missing? Suggestion: * {@return the {@linkplain System#identityHashCode(Object) identity hash code} for this lazy constant} src/java.base/share/classes/java/lang/LazyConstant.java line 257: > 255: > 256: /** > 257: * {@return a non-initializing string suitable for debugging} What is a 'non-initializing string'? ;) I think the second paragraph already covers this aspect, so you can leave it out here. src/java.base/share/classes/java/lang/LazyConstant.java line 275: > 273: /** > 274: * {@return a lazy constant to be computed later via the provided > 275: * {@code computingFunction}} The function computes the contents, not the LC itself, so: Suggestion: * {@return a lazy constant whose contents is to be computed later via the provided * {@code computingFunction}} src/java.base/share/classes/java/util/LazyCollections.java line 55: > 53: > 54: // Unsafe allows LazyCollection classes to be used early in the boot > sequence > 55: static final Unsafe UNSAFE = Unsafe.getUnsafe(); Suggestion: private static final Unsafe UNSAFE = Unsafe.getUnsafe(); src/java.base/share/classes/java/util/LazyCollections.java line 66: > 64: // using `elements.length`. > 65: @Stable > 66: private final int size; Is this really that important? What about the extra footprint added by the `int` field? We might have many instances of this class, but only one copy of the bytecode. src/java.base/share/classes/java/util/LazyCollections.java line 199: > 197: final int size = base.size(); > 198: subListRangeCheck(fromIndex, toIndex, size); > 199: return new ReverseOrderLazyListView<>(base.subList(size > - toIndex, size - fromIndex)); Why not this? (maybe add a comment?) Suggestion: return new ReverseOrderLazyListView<>(base.subList(fromIndex, toIndex)); src/java.base/share/classes/java/util/LazyCollections.java line 264: > 262: > 263: @Stable > 264: private final Map<K, Integer> indexMapper; This index mapper is a clever solution that avoids implementing a hashing function again. Lookups through this map can be folded because it is created using `Map.ofEntries`. I think you should put that in a comment here as well. src/java.base/share/classes/java/util/LazyCollections.java line 433: > 431: @Override public V getValue() { > 432: final int index = map.indexFor(getKey); > 433: final V v = map.getAcquire(getKey); I suppose you could use `orElseCompute` here, since you already have the index src/java.base/share/classes/java/util/LazyCollections.java line 488: > 486: static final class Mutexes { > 487: > 488: static final Object TOMB_STONE = new Object(); Suggestion: private static final Object TOMB_STONE = new Object(); src/java.base/share/classes/java/util/LazyCollections.java line 578: > 576: if (t == null) { > 577: final T newValue = switch (functionHolder.function()) { > 578: case Supplier<?> sup -> (T) sup.get(); Is the held function ever a Supplier? I don't see a FunctionHolder being created with one. src/java.base/share/classes/java/util/LazyCollections.java line 607: > 605: assert Thread.holdsLock(mutex) : index + "didn't hold " + mutex; > 606: // We know we hold the monitor here so plain semantic is enough > 607: if (array[index] == null) { This should always be true when we get here, right? src/java.base/share/classes/java/util/List.java line 1222: > 1220: * <p> > 1221: * Any {@link List#subList(int, int) subList()} or {@link > List#reversed()} views > 1222: * of the returned list are also lazily computed. Is this really about the views itself? Or about the elements? (AFAIK these views are typically lazily computed/created for others List impls as well) Suggestion: * The elements of any {@link List#subList(int, int) subList()} or {@link List#reversed()} views * of the returned list are also lazily computed. src/java.base/share/classes/java/util/Map.java line 1765: > 1763: * at most once per key, even in a multi-threaded environment. > Competing > 1764: * threads accessing a value already under computation will block > until an element > 1765: * is computed or an exception is thrown by the computing thread. I think technically it's more correct to say something like '... or the computing function completes abnormally'. Since, if an exception is throw inside the computing function (by the computing thread), it may be caught and handled inside the computing function as well. src/java.base/share/classes/java/util/Map.java line 1778: > 1776: * <p> > 1777: * Any {@link Map#values()} or {@link Map#entrySet()} views of the > returned map are > 1778: * also lazily computed. Suggestion: * The values of any {@link Map#values()} or {@link Map#entrySet()} views of the returned map are * also lazily computed. src/java.base/share/classes/java/util/Map.java line 1817: > 1815: final Set<K> keyCopies = Set.copyOf(keys); > 1816: Objects.requireNonNull(computingFunction); > 1817: if (keys instanceof EnumSet<?> && !keys.isEmpty()) { The fact that `keys` is being used here and not `keyCopies` looks a bit fishy. In general we should use the validated copy of our data in subsequent processing. Since `EnumSet` is mutable, it seems that these two could go out of sync. ------------- PR Review: https://git.openjdk.org/jdk/pull/27605#pullrequestreview-3448944313 PR Review Comment: https://git.openjdk.org/jdk/pull/27605#discussion_r2518699095 PR Review Comment: https://git.openjdk.org/jdk/pull/27605#discussion_r2518745565 PR Review Comment: https://git.openjdk.org/jdk/pull/27605#discussion_r2518765422 PR Review Comment: https://git.openjdk.org/jdk/pull/27605#discussion_r2518768972 PR Review Comment: https://git.openjdk.org/jdk/pull/27605#discussion_r2518779499 PR Review Comment: https://git.openjdk.org/jdk/pull/27605#discussion_r2518885149 PR Review Comment: https://git.openjdk.org/jdk/pull/27605#discussion_r2518897461 PR Review Comment: https://git.openjdk.org/jdk/pull/27605#discussion_r2518919343 PR Review Comment: https://git.openjdk.org/jdk/pull/27605#discussion_r2518968294 PR Review Comment: https://git.openjdk.org/jdk/pull/27605#discussion_r2518950708 PR Review Comment: https://git.openjdk.org/jdk/pull/27605#discussion_r2518970767 PR Review Comment: https://git.openjdk.org/jdk/pull/27605#discussion_r2519007486 PR Review Comment: https://git.openjdk.org/jdk/pull/27605#discussion_r2519002745 PR Review Comment: https://git.openjdk.org/jdk/pull/27605#discussion_r2518813240 PR Review Comment: https://git.openjdk.org/jdk/pull/27605#discussion_r2518832566 PR Review Comment: https://git.openjdk.org/jdk/pull/27605#discussion_r2518837814 PR Review Comment: https://git.openjdk.org/jdk/pull/27605#discussion_r2518880555
