On Tue, 28 Apr 2026 09:21:54 GMT, Per Minborg <[email protected]> wrote:
>> Implement JEP 531: Lazy Constants (Third Preview) >> >> This PR replaces _draft_ https://github.com/openjdk/jdk/pull/29507 >> >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > Per Minborg has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 60 additional > commits since the last revision: > > - Merge branch 'master' into > implement-lazy-constants-third-prewiew-exceptions > - Merge branch 'openjdk:master' into master > - Improve TestSegmentOffset and convert to junit > - Address comments in PR > - Change the way throwing toString is handled > - Improve LazyMap tests > - Harden tests > - Update code style in if branch > - Reorder modifiers > - Merge branch 'master' into > implement-lazy-constants-third-prewiew-exceptions > - ... and 50 more: https://git.openjdk.org/jdk/compare/bcdbf7a5...f67028cd Looks good. Left some suggestions inline. src/java.base/share/classes/java/lang/LazyConstant.java line 235: > 233: * section. > 234: * > 235: * @throws NoSuchElementException if this lazy constant is in an > error state I wonder if it's useful to add a corresponding `throws` clause to the method declaration. I thought javadoc checked this as well, but maybe that's just for checked exceptions. src/java.base/share/classes/java/lang/LazyConstant.java line 300: > 298: * : LazyConstant.of(computingFunction); > 299: * } > 300: * } This section doesn't feel needed to me. Whether the factory is idempotent or not seems like a pretty insignificant implementation detail, and the javadoc already states that a new instance is returned. src/java.base/share/classes/jdk/internal/lang/LazyConstantImpl.java line 141: > 139: if (cf != null) { > 140: return (cf instanceof Supplier<?> supplier) > 141: ? "computing function=" + supplier Shouldn't this also call `isolateToString`? test/jdk/java/lang/LazyConstant/LazyConstantTest.java line 228: > 226: entered.countDown(); > 227: try { > 228: assertTrue(release.await(TIME_OUT_S, > TimeUnit.SECONDS)); Might need to scale the timeouts here as well. test/jdk/java/lang/LazyConstant/LazyConstantTest.java line 236: > 234: > 235: var f1 = CompletableFuture.supplyAsync(constant::get, > testExecutor); > 236: assertTrue(entered.await(5, TimeUnit.SECONDS)); Suggestion: assertTrue(entered.await(TIME_OUT_S, TimeUnit.SECONDS)); test/jdk/java/lang/LazyConstant/LazyConstantTest.java line 249: > 247: assertTrue(competing.await(TIME_OUT_S, TimeUnit.SECONDS)); > 248: // While computation is blocked, only one thread should have > entered supplier > 249: Thread.sleep(OVERLAP_TIME_MS); If you do `competing.countDown()` after calling `constant.get`, I don't think you need a sleep here. test/jdk/java/lang/LazyConstant/LazyListTest.java line 622: > 620: > 621: // Javadoc equivalent > 622: class LazyList<E> extends AbstractList<E> { Seems to be unused? test/jdk/java/lang/LazyConstant/LazyMapTest.java line 309: > 307: ref.set(lazy); > 308: var x = assertThrows(NoSuchElementException.class, () -> > lazy.get(key)); > 309: assertTrue(cnt.get() > 0); Should be exactly 1 right? test/jdk/java/lang/LazyConstant/LazyMapTest.java line 495: > 493: void nullResult() { > 494: var lazy = Map.ofLazy(Set.of(0), _ -> null); > 495: var x = assertThrows(NoSuchElementException.class, () -> > lazy.getOrDefault(0, 1));; Suggestion: var x = assertThrows(NoSuchElementException.class, () -> lazy.getOrDefault(0, 1)); test/jdk/java/util/LazyCollections/ThrowablesClassUnloading.java line 56: > 54: TestState state = createFailedLazyList(); > 55: > 56: if (!waitFor(() -> state.loaderRef().refersTo(null), 4_000L)) { Timeout might need to be scaled. test/jdk/java/util/LazyCollections/ThrowablesClassUnloading.java line 113: > 111: } > 112: > 113: private static void compile(Path sourceDir, Path classesDir) throws > Exception { Can't we let jtreg compile the sources? You can grab the classes that jtreg compiled with an `@build` tag from the file system using the `test.class.path` property. test/jdk/java/util/LazyCollections/ThrowablesClassUnloading.java line 139: > 137: try { > 138: list.get(0); > 139: throw new AssertionError("Expected lazy access to fail"); Use `assertThrows`? test/jdk/java/util/LazyCollections/ThrowablesClassUnloading.java line 166: > 164: } catch (InterruptedException ignored) { > 165: } > 166: } while (System.currentTimeMillis() < deadline); I think the `waitForReferenceProcessing` whitebox API does what you want to do here. ------------- Marked as reviewed by jvernee (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/30194#pullrequestreview-4189627339 PR Review Comment: https://git.openjdk.org/jdk/pull/30194#discussion_r3154839900 PR Review Comment: https://git.openjdk.org/jdk/pull/30194#discussion_r3154858408 PR Review Comment: https://git.openjdk.org/jdk/pull/30194#discussion_r3154931308 PR Review Comment: https://git.openjdk.org/jdk/pull/30194#discussion_r3155313292 PR Review Comment: https://git.openjdk.org/jdk/pull/30194#discussion_r3155324589 PR Review Comment: https://git.openjdk.org/jdk/pull/30194#discussion_r3155348937 PR Review Comment: https://git.openjdk.org/jdk/pull/30194#discussion_r3155628097 PR Review Comment: https://git.openjdk.org/jdk/pull/30194#discussion_r3155648124 PR Review Comment: https://git.openjdk.org/jdk/pull/30194#discussion_r3155655479 PR Review Comment: https://git.openjdk.org/jdk/pull/30194#discussion_r3155536078 PR Review Comment: https://git.openjdk.org/jdk/pull/30194#discussion_r3155527394 PR Review Comment: https://git.openjdk.org/jdk/pull/30194#discussion_r3155543532 PR Review Comment: https://git.openjdk.org/jdk/pull/30194#discussion_r3155557097
