On Mon, 21 Sep 2020 05:42:29 GMT, David Holmes <dhol...@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
