RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v6]

[David Holmes]

On 16/10/2021 8:26 am, Daniel D.Daugherty wrote:
> On Fri, 15 Oct 2021 06:34:42 GMT, David Holmes <dholmes at openjdk.org> wrote:
> 
>>> Daniel D. Daugherty has updated the pull request incrementally with one additional commit since the last revision:
>>>
>>>    8249004.cr1.patch
>>
>> src/hotspot/share/prims/jvmtiEventController.cpp line 623:
>>
>>> 621:     // If we have a JvmtiThreadState, then we've reached the point where
>>> 622:     // threads can exist so create a ThreadsListHandle to protect them.
>>> 623:     ThreadsListHandle tlh;
>>
>> Good catch on the missing TLH for this code.
> 
> It wasn't quite missing from the baseline code. This version of execute():
> 
> `Handshake::execute(HandshakeClosure* hs_cl, JavaThread* target)`
> 
> used to always create a ThreadsListHandle. I added a `ThreadsListHandle*`
> parameter to that version and created a wrapper with the existing signature
> to pass `nullptr` to the execute() version with the `ThreadsListHandle*`
> parameter. What that means is that all existing callers of:
> 
> `Handshake::execute(HandshakeClosure* hs_cl, JavaThread* target)`
> 
> no longer had a ThreadsListHandle created for them. With the new sanity
> check in place, I shook the trees to make sure that we had explicit
> ThreadsListHandles in place for the locations that needed them.
> 
> `JvmtiEventControllerPrivate::recompute_enabled()` happened to be
> one of the places where the ThreadsListHandle created by execute()
> was hiding the fact that `recompute_enabled()` needed one.

Yup and that is exactly why I said good catch on finding the missing TLH.

Cheers,
David

>> src/hotspot/share/prims/jvmtiEventController.cpp line 624:
>>
>>> 622:     // threads can exist so create a ThreadsListHandle to protect them.
>>> 623:     ThreadsListHandle tlh;
>>> 624:     for (; state != NULL; state = state->next()) {
>>
>> s/NULL/nullptr/
> 
> Missed that one. Fixed.
> 
>> src/hotspot/share/runtime/handshake.cpp line 361:
>>
>>> 359:   } else {
>>> 360:     if (tlh_p == nullptr) {
>>> 361:       guarantee(Thread::is_JavaThread_protected_by_my_ThreadsList(target),
>>
>> This should be an assert once this has had some bake time.
> 
> Agreed. All of the `guarantee(Thread::is_JavaThread_protected_by_my_ThreadsList(),...`
> calls should be changed to asserts down the road.
> 
> -------------
> 
> PR: https://git.openjdk.java.net/jdk/pull/4677
>