On Thu, 20 Nov 2025 20:12:11 GMT, Chris Plummer <[email protected]> wrote:
>> 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:
>>
>> 1. Freeing the ThreadNode after handling a THREAD_START event for a virtual
>> thread.
>> 2. Doing a "GC" of virtual thread ThreadNodes just before doing a "suspend
>> all"
>>
>> 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.
>
> Chris Plummer has updated the pull request incrementally with one additional
> commit since the last revision:
>
> improve comment
Looks good to me.
Added some minor notes
src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c line 582:
> 580: /*
> 581: * Although at first it might seem that a non-zero suspendCount
> would require
> 582: * keeping the node, we don't have too if node->suspendCount ==
> suspendAllCount,
Suggestion:
* keeping the node, we don't have to if node->suspendCount ==
suspendAllCount,
src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c line 592:
> 590: }
> 591:
> 592: // All of the following conditions must pass if we are to free this
> node. Note
Suggestion:
// All of the following conditions must be met to free this node. Note
src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c line 628:
> 626:
> 627: ThreadList *list = &runningVThreads;
> 628: ThreadNode *node = list->first;
Suggestion:
ThreadNode *node = runningVThreads.first;
-------------
PR Review: https://git.openjdk.org/jdk/pull/28211#pullrequestreview-3490828800
PR Review Comment: https://git.openjdk.org/jdk/pull/28211#discussion_r2548270211
PR Review Comment: https://git.openjdk.org/jdk/pull/28211#discussion_r2548280293
PR Review Comment: https://git.openjdk.org/jdk/pull/28211#discussion_r2548283759
