On Fri, 15 Oct 2021 06:54:10 GMT, David Holmes <dhol...@openjdk.org> wrote:
> 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?
Hmmm... This is a really good observation. When I was putting together this
patch (the second time), my criteria was fairly simple: if we were calling
`Handshake::execute()` and we happened to have a `ThreadsListHandle`
in hand, then I switched to the new version of `Handshake::execute()` that
takes a `ThreadsListHandle*`.
As for what difference does it make, when we pass in a `ThreadsListHandle`,
then we're saying "This is the `ThreadsListHandle` that protects the target
JavaThread." and that's the _only_ `ThreadsListHandle` that we search. If we
call the `Handshake::execute()` form that does not take a `ThreadsListHandle`,
then we might end up searching all of the `ThreadsListHandles` that are
associated with the calling thread. Since we stop on first match, we might only
search the first one, but if the target JavaThread isn't in the first one, then
we'll
continue to search the next one and so on.
Of course, down the road, if we end up changing code like this:
guarantee(Thread::is_JavaThread_protected_by_my_ThreadsList(target),
"missing ThreadsListHandle in calling context.");
into an assert(), then we won't be doing any search in 'release' bits and then
the `Handshake::execute()` that takes a `ThreadsListHandle*` will be more
expensive in release bits.
-------------
PR: https://git.openjdk.java.net/jdk/pull/4677
