On Tue, 28 Mar 2023 19:36:18 GMT, Alan Bateman <[email protected]> wrote:
>> JEP 444 proposes to make virtual threads a permanent feature in Java 21. The
>> APIs that were preview APIs in Java 19/20 are changed to permanent and their
>> `@since`/equivalent are changed to 21 (as per the guidance in JEP 12). The
>> JNI and JVMTI versions are bumped as this is the first change in 21 to need
>> the new version number. A lot of tests are updated to drop `@enablePreview`
>> and --enable-preview.
>>
>> There is one API change from Java 19/20, the preview API
>> Thread.Builder.allowSetThreadLocals(boolean) is dropped. This requires an
>> update to the JVMTI GetThreadInfo implementation to read the TCCL
>> consistently.
>>
>> In addition, there are a small number of implementation changes to sync up
>> from the loom fibers branch:
>>
>> - A number of stack frames are `@Hidden` to reduce noise in the stack
>> traces. This exposed a few issues with the stack walker code. More
>> specifically, the cases where end of a continuation falls precisely at the
>> end of the batch, or where the remaining frames are hidden, weren't handled
>> correctly.
>> - The code to emit the JFR jdk.ThreadSleepEvent is refactored so it's in
>> Thread rather than in two classes.
>> - A few robustness improvements for OOME and SOE. There is more to do here,
>> for future PRs.
>> - New system property to print a stack trace when a virtual thread sets its
>> own value of a TL.
>> - ThreadPerTaskExecutor is changed to use FutureTask.
>>
>> Testing: tier1-6.
>
> Alan Bateman 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 four additional
> commits since the last revision:
>
> - ThreadSleepEvent refactoring
> - Merge
> - Merge
> - Initial sync from fibers branch
Changes requested by lmesnik (Reviewer).
src/java.base/share/classes/java/lang/System.java line 2566:
> 2564:
> 2565: public <V> V executeOnCarrierThread(Callable<V> task)
> throws Exception {
> 2566: if (Thread.currentThread() instanceof VirtualThread
> vthread) {
Any specific reason to don't use Thread.currentThread().isVirtual()?
test/hotspot/jtreg/runtime/vthread/JNIMonitor/JNIMonitor.java line 31:
> 29: * @summary Tests that JNI monitors work correctly with virtual threads
> 30: * @library /test/lib
> 31: * @compile JNIMonitor.java
I think that test file is compiled implicitly. So this line could be just
removed.
The same is for all other similar tests.
test/jdk/com/sun/jdi/TestScaffold.java line 980:
> 978:
> 979: if (wrapper.equals("Virtual")) {
> 980: threadFactory = Thread.ofVirtual().factory();
Should be line
469: argInfo.targetVMArgs += "--enable-preview ";
removed also?
test/jdk/com/sun/management/ThreadMXBean/VirtualThreads.java line 143:
> 141: long[] tids = new long[] { tid0, tid1 };
> 142: long[] cpuTimes = bean.getThreadCpuTime(tids);
> 143: if (Thread.currentThread().isVirtual()) {
How it worked before?
test/jdk/java/lang/Thread/virtual/GetStackTrace.java line 30:
> 28: * @requires vm.continuations
> 29: * @modules java.base/java.lang:+open
> 30: * @run testng/othervm -XX:+UnlockDiagnosticVMOptions
> -XX:+ShowHiddenFrames GetStackTrace
shouldn't be main/othervm ?
test/jdk/java/lang/Thread/virtual/VirtualThreadPinnedEventThrows.java line 29:
> 27: * @modules java.base/jdk.internal.event
> 28: * @compile/module=java.base
> jdk/internal/event/VirtualThreadPinnedEvent.java
> 29: * @run junit VirtualThreadPinnedEventThrows
Shouldn't be 'junit/othervm' used here to ensure that updated
VirtualThreadPinnedEvent is used? I don't know these details of jtreg.
test/jdk/jdk/incubator/concurrent/StructuredTaskScope/PreviewFeaturesNotEnabled.java
line 2:
> 1: /*
> 2: * Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved.
Not sure I understand what happens. The file is
test/jdk/jdk/incubator/concurrent/StructuredTaskScope/PreviewFeaturesNotEnabled.java
while it contains is public class VirtualThreadPinnedEvent. Copied by mistake?
-------------
PR Review: https://git.openjdk.org/jdk/pull/13203#pullrequestreview-1361847681
PR Review Comment: https://git.openjdk.org/jdk/pull/13203#discussion_r1151259675
PR Review Comment: https://git.openjdk.org/jdk/pull/13203#discussion_r1151175072
PR Review Comment: https://git.openjdk.org/jdk/pull/13203#discussion_r1151168150
PR Review Comment: https://git.openjdk.org/jdk/pull/13203#discussion_r1151168861
PR Review Comment: https://git.openjdk.org/jdk/pull/13203#discussion_r1151112603
PR Review Comment: https://git.openjdk.org/jdk/pull/13203#discussion_r1151153758
PR Review Comment: https://git.openjdk.org/jdk/pull/13203#discussion_r1151119165
