On Mon, 2 Oct 2023 23:11:01 GMT, Serguei Spitsyn <sspit...@openjdk.org> wrote:
> The JVMTI VirtualThreadStart events have to follow the ThreadStart events
> pattern and so, should not be thread-filtered.
> The fix includes:
> - `jvmti.xml`: remov the attribute `filtered="thread"` in the
> `VirtuallThreadStart` event spec
> - `jvmtiEventController.cpp`: remove the `VTHREAD_START_BIT` from the
> `THREAD_FILTERED_EVENT_BITS` mask and and it to the `NEED_THREAD_LIFE_EVENTS`
> mask
> - `jvmtiExport.cpp`: rearrangements in the
> `JvmtiExport::post_vthread_start()` function
>
> The fix also includes a couple of minor unification tweaks:
> - to align `JvmtiExport::post_thread_end()` with
> `JvmtiExport::post_vthread_end()` which have a little bit more optimized
> check for the `JVMTI_PHASE_PRIMORDIAL`.
> - to rename the local variable `cur_thread` as `thread` to follow the
> common pattern in `JvmtiExport::post_vthread_start()` and
> `JvmtiExport::post_vthread_end()`
>
> Testing: ran mach5 tiers 1-6. All tests are passed.
Changes requested by lmesnik (Reviewer).
src/hotspot/share/prims/jvmtiExport.cpp line 1552:
> 1550: JvmtiEnvThreadStateIterator it(state);
> 1551: for (JvmtiEnvThreadState* ets = it.first(); ets != nullptr; ets =
> it.next(ets)) {
> 1552: JvmtiEnv *env = ets->get_env();
This change as well as renaming cur_thread are not related to the main issue.
It would be better to separate them. Easier to track and backport if needed.
They are mentioned in PR but not in jira bug, hard to find the reason without
GitHub. Might be better to copy them in the bug if you want to keep them.
src/hotspot/share/prims/jvmtiExport.cpp line 1582:
> 1580: // Do not post virtual thread start event for hidden java thread.
> 1581: if
> (JvmtiEventController::is_enabled(JVMTI_EVENT_VIRTUAL_THREAD_START) &&
> 1582: !thread->is_hidden_from_external_view()) {
Do we need this check? I'm not sure that JavaThread executing a virtual thread.
Might be better to replace it with assertion?
-------------
PR Review: https://git.openjdk.org/jdk/pull/16019#pullrequestreview-1658552952
PR Review Comment: https://git.openjdk.org/jdk/pull/16019#discussion_r1346517489
PR Review Comment: https://git.openjdk.org/jdk/pull/16019#discussion_r1346513930
