On Tue, 7 Apr 2026 05:39:16 GMT, Leonid Mesnik <[email protected]> wrote:
>> The vmTestbase/nsk/jvmti/scenarios/sampling is updated to run testing in the
>> virtual threads when virtual thread test factory is enabled.
>>
>> The ThreadWrapper is helper class that allows tests execute not only main
>> thread but other threads as virtual threads.
>> It is useful for the pattern
>> `class MyThread extends Thread {...}`
>> The plan is to convert more tests focusing on serviceability tests. Helps
>> to run more jvmti functions for virtual threads.
>>
>> Please note that some tests are still not updated because it requires
>> significant efforts to redesign them. The goal is to update the only simple
>> cases.
>
> Leonid Mesnik has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - updated doc for ThreadWrapper
> - reverted some tests that can't be really run with virtual threads
## Review Summary
This PR introduces a `ThreadWrapper` class to adapt existing JVMTI test
scenarios to support virtual threads, mechanically converting 77 test files
from `extends Thread` to `extends ThreadWrapper`.
**2 findings (1 Critical, 1 Suggestion):**
1. 🔴 **Critical:** `sp05t002.java` was converted to use `ThreadWrapper`, but
the corresponding native code `sp05t002.cpp` was **not** updated. The JNI
`IsSameObject` comparisons will never match because the native code treats
`sp05t002Thread[]` elements as `jthread`, but they are now `ThreadWrapper`
instances (not `java.lang.Thread`). The test will unconditionally fail. Fix:
follow the same pattern as `sp01t001` — add a separate `Thread[]` field and
update the C++ `THREAD_CLASS_NAME` to `java/lang/Thread`.
2. 💡 **Suggestion:** `ThreadWrapper.java` class comment has three typos:
"help"→"helps", "adopt"→"adapt", "exiting"→"existing".
Overall, the conversion is mechanically consistent across the 77 files. The
`sp05t002` case is the only file with native code that was missed.
_Reviewed by glm-5.1 via Qwen Code /review_
test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/sampling/SP05/sp05t002.java
line 124:
> 122:
> 123: // basic class for tested threads
> 124: abstract class sp05t002Thread extends ThreadWrapper {
**[Critical]** `sp05t002Thread` now extends `ThreadWrapper` instead of
`Thread`, but the corresponding native code in `sp05t002.cpp` was **not**
updated.
The native code still uses:
#define THREAD_CLASS_NAME "nsk/jvmti/scenarios/sampling/SP05/sp05t002Thread"
#define THREADS_FIELD_SIG "[L" THREAD_CLASS_NAME ";"
Since `sp05t002Thread` is now a `ThreadWrapper` (which holds a `Thread`
internally but is NOT itself a `java.lang.Thread`), the JNI `IsSameObject`
comparisons in `callbackThreadStart`/`callbackThreadEnd` will never match
against JVMTI-provided `java.lang.Thread` references. The test will
unconditionally fail.
**Suggested fix:** Apply the same pattern used for `sp01t001`:
1. Add a `static Thread threads[]` field populated with `getThread()` calls
from the ThreadWrapper instances
2. In `sp05t002.cpp`, change `THREAD_CLASS_NAME` to `"java/lang/Thread"` and
update `THREADS_FIELD_SIG` accordingly
test/lib/jdk/test/lib/thread/ThreadWrapper.java line 33:
> 31: This class help to adopt exiting multithreaded tests with following
> pattern:
> 32:
> 33: resumethrd02Thread extends Thread {...}
**[Suggestion]** The class-level comment has three word errors that make it
misleading:
- `"help"` → `"helps"` (subject-verb agreement: "this class helps")
- `"adopt"` → `"adapt"` (the class adapts existing tests, not adopts them)
- `"exiting"` → `"existing"` (refers to tests that already exist, not ones that
are exiting/terminating)
Suggested rephrase: `This class helps to adapt existing multithreaded tests
with the following pattern:`
-------------
Changes requested by swen (Committer).
PR Review: https://git.openjdk.org/jdk/pull/30591#pullrequestreview-4065072542
PR Review Comment: https://git.openjdk.org/jdk/pull/30591#discussion_r3042212036
PR Review Comment: https://git.openjdk.org/jdk/pull/30591#discussion_r3042212554
