8343177: JFR: Remove critical section for thread id assignment #21756

mgronlun · 2024-10-29T10:28:45Z

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

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8343177: JFR: Remove critical section for thread id assignment (Enhancement - P3)

Reviewers

David Holmes (@dholmes-ora - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/21756/head:pull/21756
$ git checkout pull/21756

Update a local copy of the PR:
$ git checkout pull/21756
$ git pull https://git.openjdk.org/jdk.git pull/21756/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21756

View PR using the GUI difftool:
$ git pr show -t 21756

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/21756.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2024-10-29T10:29:23Z

👋 Welcome back mgronlun! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-10-29T10:29:56Z

@mgronlun This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8343177: JFR: Remove critical section for thread id assignment

Reviewed-by: dholmes

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 70 new commits pushed to the master branch:

f77a514: 8343194: Avoid redundant Hashtable.containsKey call in CodeSource.readObject
4a70c83: 8343378: Exceptions in javax/management DeadLockTest.java do not cause test failure
8d4d589: 8343297: Vector unsigned min/max test are failing with -Xcomp
2a4d9d9: 8343379: [BACKOUT] 8343190 GHA: Try building JTReg several times
6f6cfe6: 8343380: C2: assert(iff->in(1)->is_OpaqueNotNull()) failed: must be OpaqueNotNull
cbda758: 8343122: RISC-V: C2: Small improvement for real runtime callouts
803612e: 8343121: RISC-V: More accurate max size for C2SafepointPollStub and C2EntryBarrierStub
b627a41: 8343345: Use -jvmArgsPrepend when running microbenchmarks in RunTests.gmk
2932144: 8343305: Remove Indify-dependent microbenchmarks
34655c6: 8342544: [macos] jpackage test helper should check for both "--app-image" and "--mac-sign" for signing predefined app image case
... and 60 more: https://git.openjdk.org/jdk/compare/d5fb6b4a3cf4926acb333e7ee55f96fc76225631...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2024-10-29T10:30:43Z

@mgronlun The following label will be automatically applied to this pull request:

hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2024-10-29T10:33:46Z

Webrevs

mgronlun · 2024-10-29T10:42:32Z

/remove hotspot

openjdk · 2024-10-29T10:42:44Z

@mgronlun Unknown command remove - for a list of valid commands use /help.

mgronlun · 2024-10-29T10:43:05Z

/help

openjdk · 2024-10-29T10:43:43Z

@mgronlun Available commands:

approval - request for maintainer's approval
approve - null
author - sets an overriding author to be used in the commit when the PR is integrated
backport - create a backport
cc - add or remove an additional classification label
clean - Mark the backport pull request as a clean backport
contributor - adds or removes additional contributors for a PR
covered - used when employer has signed the OCA
csr - require a compatibility and specification request (CSR) for this pull request
help - shows this text
integrate - performs integration of the changes in the PR
issue - edit the list of issues that this PR solves
jep - require a JDK Enhancement Proposal (JEP) for this pull request
label - add or remove an additional classification label
open - Set the pull request state to "open"
reviewer - manage additional reviewers for a PR
reviewers - set the number of additional required reviewers for this PR
signed - used after signing the OCA
solves - edit the list of issues that this PR solves
sponsor - performs integration of a PR that is authored by a non-committer
summary - updates the summary in the commit message
test - used to run tests

mgronlun · 2024-10-29T10:44:00Z

/label remove hotspot

openjdk · 2024-10-29T10:44:27Z

@mgronlun
The hotspot label was successfully removed.

mgronlun · 2024-10-29T10:44:35Z

/list add hotspot-jfr

openjdk · 2024-10-29T10:44:56Z

@mgronlun Unknown command list - for a list of valid commands use /help.

mgronlun · 2024-10-29T10:45:18Z

/label add hotspot-jfr

mgronlun · 2024-10-29T10:45:30Z

/label add hotspot-runtime

openjdk · 2024-10-29T10:46:01Z

@mgronlun
The hotspot-jfr label was successfully added.

openjdk · 2024-10-29T10:46:29Z

@mgronlun
The hotspot-runtime label was successfully added.

dholmes-ora

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?

dholmes-ora · 2024-10-30T05:28:28Z

src/hotspot/share/runtime/thread.hpp

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


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

I changed this with the argument that it is unbalanced to have a non-static instance function available on all Threads for calling t->set_as_starting_thread(), which should only be called on a JavaThread, to set a static field, a static field that is only set once, by the main (primordial) JavaThread during startup in Threads::create_VM().

The assert function is_starting_thread(Thread*t) is for asserting the special assignment of 1 to the primordial thread.

Okay so nothing inherently related to the critical section removal, just an API cleanup.

Actually the more I look at this the more I dislike the fact set_as_starting_thread also calls os::create_main_thread. Seems to me that should be done directly by the caller, and if successful we call set_as_starting_thread. But that is a different cleanup.

Just FTR set_as_starting_thread originally only called os::create_main_thread so why it even existed is a bit of a mystery. The actual _starting_thread variable was something I added under JDK-8214097.

dholmes-ora · 2024-10-30T05:33:35Z

src/hotspot/share/jfr/support/jfrThreadLocal.cpp

  assert(t != nullptr, "invariant");
  assert(t->is_Java_thread(), "invariant");
  oop threadObj = JavaThread::cast(t)->threadObj();
  return threadObj != nullptr ? AccessThreadTraceId::id(threadObj) : 0;
 }

+#ifdef ASSERT
+static bool assignment_precondition(const Thread* t, JfrThreadLocal* tl) {


Suggested change

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

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

Good suggestion.

dholmes-ora · 2024-10-30T05:35:09Z

src/hotspot/share/jfr/support/jfrThreadLocal.cpp

+  assert(jt != nullptr, "invariant");
+  assert(Thread::is_starting_thread(jt), "invariant");
+  assert(jt->threadObj() == nullptr, "invariant");
+  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.

If the primordial thread ID assignment is changed from 1 to 3, then that will have to be updated, of course.

Hopefully the value will be available via the ObjectMonitor API so this code can use it symbolically.

What happens to ID 1 and 2? Other threads that need to be tracked, and receive special treatment in a similar manner?

There will be no Java thread-id 1 and 2. Those values are used as values for deflated-marker and anonymous-owner for ObjectMonitor see #21565

OK, they are reserved constants that do not denote a thread but particular states.

That's okay from our perspective.

mgronlun · 2024-10-30T11:27:54Z

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?

A traceid is a unique, monotonic, type specific ID that JFR assigns to artifacts (instances) (Threads, Klasses, Methods, CLDs, Symbols etc).

Invariant; no reuse of IDs.

With Loom, the unique monotonic ID for threads has been made intrinsic to the tid field of the threadObj.

dholmes-ora

Overall looks good. Thanks for the clarifications.

I have one change request with regards to naming below.

Thanks

dholmes-ora · 2024-10-31T04:53:20Z

src/hotspot/share/jfr/support/jfrThreadLocal.cpp

+  assert(jt != nullptr, "invariant");
+  assert(Thread::is_starting_thread(jt), "invariant");
+  assert(jt->threadObj() == nullptr, "invariant");
+  jt->jfr_thread_local()->_jvm_thread_id = 1;


Hopefully the value will be available via the ObjectMonitor API so this code can use it symbolically.

dholmes-ora · 2024-10-31T05:02:59Z

src/hotspot/share/runtime/thread.hpp

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


Actually the more I look at this the more I dislike the fact set_as_starting_thread also calls os::create_main_thread. Seems to me that should be done directly by the caller, and if successful we call set_as_starting_thread. But that is a different cleanup.

dholmes-ora · 2024-10-31T05:07:35Z

src/hotspot/share/jfr/jfr.cpp

+void Jfr::initialize_primordial_thread(JavaThread* jt) {
+  JfrThreadLocal::initialize_primordial_thread(jt);


The main thread is generally not the process primordial thread. Can we use main_thread or starting_thread here to keep semi-consistent with the Thread/Threads code terminology? Thanks

mgronlun · 2024-11-01T13:29:16Z

Thanks @dholmes-ora for your review.

mgronlun · 2024-11-01T14:50:53Z

/integrate

openjdk · 2024-11-01T14:51:54Z

Going to push as commit 5995786.
Since your change was applied there have been 73 commits pushed to the master branch:

751a914: 8340733: Add scope for relaxing constraint on JavaCalls from CompilerThread
7e87c07: 8340116: test/jdk/sun/security/tools/jarsigner/PreserveRawManifestEntryAndDigest.java can fail due to regex
da0e9e3: 8343333: Parallel: Cleanup comment referring Solaris in MutableNUMASpace
f77a514: 8343194: Avoid redundant Hashtable.containsKey call in CodeSource.readObject
4a70c83: 8343378: Exceptions in javax/management DeadLockTest.java do not cause test failure
8d4d589: 8343297: Vector unsigned min/max test are failing with -Xcomp
2a4d9d9: 8343379: [BACKOUT] 8343190 GHA: Try building JTReg several times
6f6cfe6: 8343380: C2: assert(iff->in(1)->is_OpaqueNotNull()) failed: must be OpaqueNotNull
cbda758: 8343122: RISC-V: C2: Small improvement for real runtime callouts
803612e: 8343121: RISC-V: More accurate max size for C2SafepointPollStub and C2EntryBarrierStub
... and 63 more: https://git.openjdk.org/jdk/compare/d5fb6b4a3cf4926acb333e7ee55f96fc76225631...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-11-01T14:52:03Z

@mgronlun Pushed as commit 5995786.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

dholmes-ora · 2024-11-04T02:44:25Z

@mgronlun - two reviews required for hotspot changes.

8343177

4de0844

openjdk bot added the rfr Pull request is ready for review label Oct 29, 2024

openjdk bot added the hotspot hotspot-dev@openjdk.org label Oct 29, 2024

openjdk bot removed the hotspot hotspot-dev@openjdk.org label Oct 29, 2024

openjdk bot added the hotspot-jfr hotspot-jfr-dev@openjdk.org label Oct 29, 2024

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Oct 29, 2024

mgronlun mentioned this pull request Oct 29, 2024

8337789: JEP 509: JFR CPU-Time Profiling (Experimental) #20752

Closed

3 tasks

dholmes-ora reviewed Oct 30, 2024

View reviewed changes

can_assign

c73725b

dholmes-ora suggested changes Oct 31, 2024

View reviewed changes

rename primordial to main thread

6053116

dholmes-ora approved these changes Nov 1, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 1, 2024

openjdk bot added the integrated Pull request has been integrated label Nov 1, 2024

openjdk bot closed this Nov 1, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 1, 2024

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

		void Jfr::initialize_primordial_thread(JavaThread* jt) {
		JfrThreadLocal::initialize_primordial_thread(jt);

8343177: JFR: Remove critical section for thread id assignment #21756

8343177: JFR: Remove critical section for thread id assignment #21756

Uh oh!

Conversation

mgronlun commented Oct 29, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Oct 29, 2024

Uh oh!

openjdk bot commented Oct 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Oct 29, 2024

Uh oh!

mlbridge bot commented Oct 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

mgronlun commented Oct 29, 2024

Uh oh!

openjdk bot commented Oct 29, 2024

Uh oh!

mgronlun commented Oct 29, 2024

Uh oh!

openjdk bot commented Oct 29, 2024

Uh oh!

mgronlun commented Oct 29, 2024

Uh oh!

openjdk bot commented Oct 29, 2024

Uh oh!

mgronlun commented Oct 29, 2024

Uh oh!

openjdk bot commented Oct 29, 2024

Uh oh!

mgronlun commented Oct 29, 2024

Uh oh!

mgronlun commented Oct 29, 2024

Uh oh!

openjdk bot commented Oct 29, 2024

Uh oh!

openjdk bot commented Oct 29, 2024

Uh oh!

dholmes-ora left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgronlun Oct 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgronlun Oct 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coleenp Oct 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

mgronlun commented Oct 29, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Oct 29, 2024 •

edited

Loading

mlbridge bot commented Oct 29, 2024 •

edited

Loading

mgronlun Oct 30, 2024 •

edited

Loading

mgronlun Oct 31, 2024 •

edited

Loading

coleenp Oct 31, 2024 •

edited

Loading

mgronlun commented Oct 30, 2024 •

edited

Loading