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

[David Holmes]

On Thu, 14 Oct 2021 16:03:28 GMT, Daniel D. Daugherty <dcubed at openjdk.org> wrote:

>> A fix to reduce ThreadsListHandle overhead in relation to handshakes and
>> we add sanity checks for ThreadsListHandles higher in the call stack.
>> 
>> This fix was tested with Mach5 Tier[1-8]; Tier8 is still running.
>
> Daniel D. Daugherty has updated the pull request incrementally with one additional commit since the last revision:
> 
>   8249004.cr1.patch

Hi Dan,

This looks promising but I'm unclear on some of the details. I can't quite work out the criteria for deciding when to pass the TLH through to Handshake::execute. If it is passed through then the target is checked for being alive ie covered by the TLH. But if the TLH is null then it is assumed that the target is protected elsewhere up the stack and also assumed to be alive. I'm not sure why the two must go together. For example given this code:


    ThreadsListHandle tlh(current_thread);
    JavaThread *java_thread;
    err = JvmtiExport::cv_external_thread_to_JavaThread(tlh.list(), *thread_list, &java_thread, NULL);
    GetSingleStackTraceClosure op(this, current_thread, *thread_list, max_frame_count);
    Handshake::execute(&op, &tlh, java_thread);

what difference does it make if instead the last line is just:

` Handshake::execute(&op, java_thread);`

? How do I know which form should be used?

Thanks,
David

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.

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/

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.

src/hotspot/share/runtime/thread.cpp line 1771:

> 1769:   guarantee(Thread::is_JavaThread_protected_by_my_ThreadsList(this),
> 1770:             "missing ThreadsListHandle in calling context.");
> 1771:   if (is_exiting()) {

This seems an unrelated change in behaviour ??

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

PR: https://git.openjdk.java.net/jdk/pull/4677