RFR: 8238761: Asynchronous handshakes [v3]

[Robbin Ehn]

On Mon, 21 Sep 2020 05:42:29 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> Robbin Ehn has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Removed double check, fix comment, removed not needed function, updated logs
>
> src/hotspot/share/runtime/thread.cpp line 481:
> 
>> 479: #ifdef ASSERT
>> 480: // A JavaThread is considered "dangling" if it is not the current
>> 481: // thread, his not handshaking with current thread, as been added the Threads
> 
> s/his/is/ ?
> s/as been added the/has been added to the/
> 
> But rather than describe all the conditions, few of which are actually visible in the assertion below, why not just
> rephrase in terms of the conditions checked i.e. // A JavaThread is considered dangling if it not handshake-safe with
> respect to the current thread, or // it is not on a ThreadsList.
> The is_handshake_safe_for method should describe all the conditions that make a target handshake safe.

Fixed

> src/hotspot/share/runtime/thread.cpp line 851:
> 
>> 849: bool
>> 850: JavaThread::is_thread_fully_suspended(bool wait_for_suspend, uint32_t *bits) {
>> 851:   if (this != Thread::current()) {
> 
> Why/how is a non-JavaThread calling this?

is_thread_fully_suspended() is used in JVM TI handshake and this change-set allow VM thread to execute them.
This method have no dependency that caller is a JavaThread.

> src/hotspot/share/runtime/thread.cpp line 2441:
> 
>> 2439:   assert(Thread::current()->is_VM_thread() ||
>> 2440:          is_handshake_safe_for(Thread::current()),
>> 2441:          "should be in the vm thread, self or handshakee");
> 
> This seems too general. This should either be a VMoperation or a direct handshake, but not both.

send_thread_stop() is only called from a handshake operation.
Fixed.

> src/hotspot/share/utilities/filterQueue.hpp line 57:
> 
>> 55:
>> 56:   // MT-safe
>> 57:   // Since pops and adds are allowed while we add, we do not know if _first is same even if it's the same address.
> 
> This comment seems out of context on the declaration of add, as it is describing a detail of the implementation - to
> what are we comparing _first to be the same ?? If you want to just document the MT properties abstractedly then
> describing this a lock-free would suffice. Though if pop requires locking then it's implementation is also constrained
> to work with the lock-free version of add. Overall it is unclear how documenting "external serialization needed"
> actually helps the user of this API. ??

Removed comment.
Ok. Since it's same notation as posix uses I thought was clear that any method marked MT-Unsafe is not safe call to
from a multithreaded program.

Suggestion on how to separate the MT-safe from MT-usafe methods ?

> src/hotspot/share/utilities/filterQueue.inline.hpp line 42:
> 
>> 40:       break;
>> 41:     }
>> 42:     yield.wait();
> 
> Was the need for spinwaits identified through benchmarking? Do you really expect this to be hot?

No and no.
It was added to guard against any pathologically case, such as the gtest stress-tests.

> src/hotspot/share/utilities/filterQueue.inline.hpp line 63:
> 
>> 61: }
>> 62:
>> 63: // MT-Unsafe, external serialization needed.
> 
> So IIUC this queue supports multiple concurrent add()s, but has to be restricted to a single pop() at a time (although
> that is allowed to execute concurrently with the add()s) - correct?

Yes

> test/hotspot/jtreg/runtime/handshake/MixedHandshakeWalkStackTest.java line 38:
> 
>> 36:
>> 37: public class MixedHandshakeWalkStackTest {
>> 38:     public static Thread tthreads[];
> 
> why tthreads? It looks like a typo :)

Test threads, changed to testThreads.

> test/hotspot/jtreg/runtime/handshake/AsyncHandshakeWalkStackTest.java line 30:
> 
>> 28:  * @build AsyncHandshakeWalkStackTest
>> 29:  * @run driver ClassFileInstaller sun.hotspot.WhiteBox
>> 30:  *                              sun.hotspot.WhiteBox$WhiteBoxPermission
> 
> This is not needed as ClassFileInstaller already handles WhiteBox's nested classes.

Fixed

> test/hotspot/jtreg/runtime/handshake/MixedHandshakeWalkStackTest.java line 30:
> 
>> 28:  * @build MixedHandshakeWalkStackTest
>> 29:  * @run driver ClassFileInstaller sun.hotspot.WhiteBox
>> 30:  *                              sun.hotspot.WhiteBox$WhiteBoxPermission
> 
> Not needed - see earlier comment.

Fixed

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

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