RFR: 8343177: JFR: Remove critical section for thread id assignment

[David Holmes]

On Tue, 29 Oct 2024 10:28:45 GMT, Markus Grönlund <mgronlun at openjdk.org> wrote:

> Greetings,
> 
> With Loom, JFR had to change the assignment of unique thread IDs to threads. For JavaThreads, a dependency exists on the threadObj before the thread ID can be determined and assigned. We currently have a lazy assignment scheme that assigns on first use. Several threads, such as thread iterators and sampling threads, can issue the first use. Hence, a critical section protects assignments.
> 
> However, a critical section at this location makes it challenging to build robust signal handlers, for example, because we cannot read the thread ID.
> 
> We can remove this critical section with careful rearrangements, ensuring threads are only assigned a thread ID in thread state _thread_new (a thread state that at least the JFR sampler and the JFR iterators exclude).
> 
> The problem child is JNI attaching threads, created directly into state _thread_in_vm and added to the threads list before the creation and assignment of the threadObj. We can also manage this case by being careful when we sample such a thread, alternatively allowing for a thread ID of 0 or taking account of the 'attaching via jni' property. 
> 
> Testing: jdk_jfr Tier 1- 3, Stress testing
> 
> Thanks
> Markus

Sorry I struggled to map the code changes to the description of the changes. I think I'm missing some background context here - what is the "traceid" of a thread?

src/hotspot/share/jfr/support/jfrThreadLocal.cpp line 106:

> 104:   assert(Thread::is_starting_thread(jt), "invariant");
> 105:   assert(jt->threadObj() == nullptr, "invariant");
> 106:   jt->jfr_thread_local()->_jvm_thread_id = 1;

Is this supposed to reflect the `java.lang.Thread` thread-id? If so this is changing to 3 with the object monitor virtual thread changes.

src/hotspot/share/jfr/support/jfrThreadLocal.cpp line 438:

> 436: 
> 437: #ifdef ASSERT
> 438: static bool assignment_precondition(const Thread* t, JfrThreadLocal* tl) {

Suggestion:

static bool can_assign(const Thread* t, JfrThreadLocal* tl) {

src/hotspot/share/runtime/thread.hpp line 505:

> 503:   // Sets the argument thread as starting thread. Returns failure if thread
> 504:   // creation fails due to lack of memory, too many threads etc.
> 505:   static bool set_as_starting_thread(JavaThread* jt);

Why change this? Overall I don't understand the "starting thread" changes.

-------------

PR Review: https://git.openjdk.org/jdk/pull/21756#pullrequestreview-2403653437
PR Review Comment: https://git.openjdk.org/jdk/pull/21756#discussion_r1821908468
PR Review Comment: https://git.openjdk.org/jdk/pull/21756#discussion_r1821907262
PR Review Comment: https://git.openjdk.org/jdk/pull/21756#discussion_r1821901961