8282441: [LOOM] The debug agent should attempt to free vthread ThreadNodes #28211
Conversation
|
👋 Welcome back cjplummer! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
openjdk
bot
changed the title
|
@plummercj The following label will be automatically applied to this pull request:
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. |
|
|
||
| #ifdef DEBUG_THREADNAME | ||
| { | ||
| /* Set the thread name */ | ||
| jvmtiThreadInfo info; | ||
| jvmtiError error; | ||
|
|
||
| memset(&info, 0, sizeof(info)); | ||
| error = JVMTI_FUNC_PTR(gdata->jvmti,GetThreadInfo) | ||
| (gdata->jvmti, node->thread, &info); | ||
| if (info.name != NULL) { | ||
| strncpy(node->name, info.name, sizeof(node->name) - 1); | ||
| jvmtiDeallocate(info.name); | ||
| } | ||
| } | ||
| #endif | ||
|
|
There was a problem hiding this comment.
I moved this to earlier in the function because at one point I was using the thread name in some debug printfs, and the thread name was being setup too late. I decided to keep it here just in case I need to do that again in the future.
| * this thread, so the suspend count is 0, unless it is a vthread. | ||
| * this thread, so the suspend count is 0. | ||
| */ | ||
| if (isVThread(thread)) { | ||
| jint vthread_state = 0; | ||
| jvmtiError error = threadState(thread, &vthread_state); | ||
| if (error != JVMTI_ERROR_NONE) { | ||
| EXIT_ERROR(error, "getting vthread state"); | ||
| } | ||
| if (vthread_state == 0) { | ||
| // If state == 0, then this is a new vthread that has not been started yet. | ||
| *count = 0; | ||
| } else { | ||
| // This is a started vthread that we are not tracking. Use suspendAllCount. | ||
| *count = suspendAllCount; | ||
| } | ||
| } else { | ||
| *count = 0; | ||
| } |
There was a problem hiding this comment.
This change could use some explaining to help with the review. After this change the code is back to what it was before vthread support was added. This extra work was needed because sometimes the ThreadNode for the vthread did not already exist. Since now findRunningThread() will create the ThreadNode, there is no longer a need for this code to deal with it not existing.
1 participant
This is first of what will probably be 3 PRs to improve virtual thread ThreadNode handling in the debug agent, with a goal of improving performance and reducing footprint.
This PR focuses on purging virtual thread ThreadNodes in two places:
At some point in the future I want to attempt to free the ThreadNodes after performing ThreadReference related commands, which will result in the creation of a ThreadNode if not already created. Another area is in handleReportEventCompositeCommand() when the thread is not null and we are using the SUSPEND_NONE policy. This will get more ThreadNodes freed immediately after handling an event.
Part of the challenge with this PR is that there are many places in the debug agent that expect a ThreadNode to already have been created for the virtual thread, but now it is common that they have not. The main way this has been addressed is by having findRunningThread() create the ThreadNode if it does not already exist.
At some point in the future I will probably add logic to only "GC" ThreadNodes after the number reaches some threshold, but right now I want to free them very aggressively so we'll catch any bugs where there debug agent expects a ThreadNode to exist, but possibly now it might not.
Tested by running all tier2, tier5, and tier6 CI svc tests.
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/28211/head:pull/28211$ git checkout pull/28211Update a local copy of the PR:
$ git checkout pull/28211$ git pull https://git.openjdk.org/jdk.git pull/28211/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 28211View PR using the GUI difftool:
$ git pr show -t 28211Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/28211.diff