Re: RFR: 8301380: jdk/jfr/api/consumer/streaming/TestCrossProcessStreaming.java [v2]
Greetings,
please help review this small adjustment to close the gap where iterated threads that attach via jni are left without a valid JFR thread id. For details, please see the JIRA issue.
Testing: jdk_jfr
Thanks Markus
Markus Grönlund has updated the pull request incrementally with two additional commits since the last revision: - typos - unconditional id assignment ------------- Changes: - all: https://git.openjdk.org/jdk/pull/12388/files - new: https://git.openjdk.org/jdk/pull/12388/files/d3d42150..34fc58aa Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12388&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12388&range=00-01 Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/12388.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12388/head:pull/12388 PR: https://git.openjdk.org/jdk/pull/12388
On Thu, 2 Feb 2023 15:34:14 GMT, Markus Grönlund <mgronlun@openjdk.org> wrote:
Greetings,
please help review this small adjustment to close the gap where iterated threads that attach via jni are left without a valid JFR thread id. For details, please see the JIRA issue.
Testing: jdk_jfr
Thanks Markus
Markus Grönlund has updated the pull request incrementally with two additional commits since the last revision:
- typos - unconditional id assignment
Code changes look good. A couple of suggested comment changes to explain the reasoning more clearly. Thanks. src/hotspot/share/prims/jni.cpp line 3468:
3466: static void post_thread_start_event(const JavaThread* jt) { 3467: assert(jt != nullptr, "invariant"); 3468: // We hoist the read for the thread id to ensure the thread is assigned an id (lazy assignment).
Suggestion: // Ensure the thread is assigned an id (lazy assignment) even if the event isn't committed. src/hotspot/share/prims/jni.cpp line 3841:
3839: } 3840: 3841: // Please keep this inside the _attaching_via_jni section.
Suggestion: // Ensure the thread_id is set whilst still `_attaching_via_jni` so that it can be seen // once the thread is included by the JFR thread iterator. ? ------------- Marked as reviewed by dholmes (Reviewer). PR: https://git.openjdk.org/jdk/pull/12388
On Fri, 3 Feb 2023 04:06:46 GMT, David Holmes <dholmes@openjdk.org> wrote:
Markus Grönlund has updated the pull request incrementally with two additional commits since the last revision:
- typos - unconditional id assignment
src/hotspot/share/prims/jni.cpp line 3468:
3466: static void post_thread_start_event(const JavaThread* jt) { 3467: assert(jt != nullptr, "invariant"); 3468: // We hoist the read for the thread id to ensure the thread is assigned an id (lazy assignment).
Suggestion:
// Ensure the thread is assigned an id (lazy assignment) even if the event isn't committed.
Thanks David. I realized there is no need to move anything in jni.cpp, so I rolled them back. The change to the iterator will suffice.
src/hotspot/share/prims/jni.cpp line 3841:
3839: } 3840: 3841: // Please keep this inside the _attaching_via_jni section.
Suggestion:
// Ensure the thread_id is set whilst still `_attaching_via_jni` so that it can be seen // once the thread is included by the JFR thread iterator.
?
Thanks David. I realized there is no need to move anything in jni.cpp, so I rolled them back. The change to the iterator will suffice. ------------- PR: https://git.openjdk.org/jdk/pull/12388
On Fri, 3 Feb 2023 09:28:05 GMT, Markus Grönlund <mgronlun@openjdk.org> wrote:
src/hotspot/share/prims/jni.cpp line 3841:
3839: } 3840: 3841: // Please keep this inside the _attaching_via_jni section.
Suggestion:
// Ensure the thread_id is set whilst still `_attaching_via_jni` so that it can be seen // once the thread is included by the JFR thread iterator.
?
Thanks David. I realized there is no need to move anything in jni.cpp, so I rolled them back. The change to the iterator will suffice.
It will? Don't you still have the problem that the thread can be seen after ` thread->set_done_attaching_via_jni();` but before `post_thread_start_event(thread);` where the id will be set? ------------- PR: https://git.openjdk.org/jdk/pull/12388
On Mon, 6 Feb 2023 00:49:47 GMT, David Holmes <dholmes@openjdk.org> wrote:
Thanks David. I realized there is no need to move anything in jni.cpp, so I rolled them back. The change to the iterator will suffice.
It will? Don't you still have the problem that the thread can be seen after ` thread->set_done_attaching_via_jni();` but before `post_thread_start_event(thread);` where the id will be set?
Yes. The JFR_JVM_THREAD_ID(thread) is a lazy assignment of the thread id. It can be done by the thread itself, or the periodic task thread. The premise for a JavaThread is that there exist a threadObj. ------------- PR: https://git.openjdk.org/jdk/pull/12388
On Mon, 6 Feb 2023 09:40:00 GMT, Markus Grönlund <mgronlun@openjdk.org> wrote:
It will? Don't you still have the problem that the thread can be seen after ` thread->set_done_attaching_via_jni();` but before `post_thread_start_event(thread);` where the id will be set?
Yes. The JFR_JVM_THREAD_ID(thread) is a lazy assignment of the thread id. It can be done by the thread itself, or the periodic task thread. The premise for a JavaThread is that there exist a threadObj.
Now I get it. The zero thread id only arises when the `threadObj` is null and that is no longer possible when we skip JNI-attaching threads. ------------- PR: https://git.openjdk.org/jdk/pull/12388
participants (2)
-
David Holmes
-
Markus Grönlund