On Mon, 29 Jul 2024 09:49:11 GMT, Jiawei Tang <jwt...@openjdk.org> wrote:
>> I add the testcase which can reproduce the crash. I hope that I could get
>> some advise if the codes need changing.
>
> Jiawei Tang has refreshed the contents of this pull request, and previous
> commits have been removed. The incremental views will show differences
> compared to the previous content of the PR. The pull request contains one new
> commit since the last revision:
>
> 8337331: crash: pinned virtual thread will lead to jvm crash when running
> with the javaagent option
Can we not just preload the problematic class so that it won't happen during
the transition?
test/hotspot/jtreg/serviceability/jvmti/vthread/VThreadTraceWithAgent/TestPinCaseWithTrace.java
line 2:
> 1: /*
> 2: * Copyright (c) 2024, 2024, Oracle and/or its affiliates. All rights
> reserved.
Please only use a single year here.
test/hotspot/jtreg/serviceability/jvmti/vthread/VThreadTraceWithAgent/TestPinCaseWithTrace.java
line 36:
> 34: * @run main/othervm/timeout=100 -Djdk.tracePinnedThreads=full
> TestPinCaseWithTrace
> 35: * @run main/othervm/timeout=100 -javaagent:TestPinCaseWithTrace.jar
> TestPinCaseWithTrace
> 36: * @run main/othervm/timeout=100 -Djdk.tracePinnedThreads=full
> -javaagent:TestPinCaseWithTrace.jar TestPinCaseWithTrace
Unclear why we need the three variants.
Also where does the timeout value come from? How long does the test take to run?
test/hotspot/jtreg/serviceability/jvmti/vthread/VThreadTraceWithAgent/TestPinCaseWithTrace.java
line 62:
> 60: public static void main(String[] args) throws Exception{
> 61: ExecutorService scheduler = Executors.newFixedThreadPool(1);
> 62: Thread.Builder builder =
> TestPinCaseWithTrace.virtualThreadBuilder(scheduler);
Can you not just create a Virtual Thread directly rather than defining a
single-threaded executor??
test/hotspot/jtreg/serviceability/jvmti/vthread/VThreadTraceWithAgent/libPinJNI.c
line 28:
> 26: JNIEXPORT jint JNICALL
> 27: Java_TestPinCaseWithTrace_nativeFuncPin(JNIEnv* env, jclass klass, jint
> x) {
> 28: jmethodID nativeBaz = (*env)->GetStaticMethodID(env, klass,
> "native2Java", "(I)I");
Suggestion: just use `m` rather than `nativeBaz`.
-------------
PR Review: https://git.openjdk.org/jdk/pull/20373#pullrequestreview-2204496515
PR Review Comment: https://git.openjdk.org/jdk/pull/20373#discussion_r1694947729
PR Review Comment: https://git.openjdk.org/jdk/pull/20373#discussion_r1694949670
PR Review Comment: https://git.openjdk.org/jdk/pull/20373#discussion_r1694952022
PR Review Comment: https://git.openjdk.org/jdk/pull/20373#discussion_r1694954328
