On Tue, 28 Mar 2023 21:36:04 GMT, Leonid Mesnik <lmes...@openjdk.org> wrote:
>> 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
>
> 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.
Most of the test changes were just a few sed commands remove lines with
`@enablePreview`, remove `--enable-preview -source ${jdk.version}` from
`@compile` tags. We can remove the `@compile` tags that don't specify any other
options as they aren't needed now, up to you.
> 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?
tid0 is the thread ID of a platform therad. tid1 is the threadID of a virtual
thread. The only change here is allow this test run with the main wrapper
plugin
([CODETOOLS-7903373](https://bugs.openjdk.org/browse/CODETOOLS-7903373)), it
would otherwise have to be excluded for those runs.
> 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 ?
You're right, this came over from the loom repo and I didn't know that it
wasn't running (no @Test). I think it would be better to run this test without
ShowHiddenFrames, it's just need to known the expected bottom most frame.
> 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.
This is using the jtreg support for patching system modules. jtreg runs the
test in a new VM with --patch-module.
> 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?
Thank, I'm not sure what happened there as this test is deleted.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/13203#discussion_r1151507288
PR Review Comment: https://git.openjdk.org/jdk/pull/13203#discussion_r1151502964
PR Review Comment: https://git.openjdk.org/jdk/pull/13203#discussion_r1151500272
PR Review Comment: https://git.openjdk.org/jdk/pull/13203#discussion_r1151495733
PR Review Comment: https://git.openjdk.org/jdk/pull/13203#discussion_r1151496227
