RFR 8240918: [REDO] Allow direct handshakes without VMThread intervention

Daniel D. Daugherty daniel.daugherty at oracle.com
Thu Apr 2 17:04:22 UTC 2020


> Full: http://cr.openjdk.java.net/~pchilanomate/8240918/v2/webrev/ 

Thanks for redoing to this patch!


src/hotspot/share/runtime/handshake.hpp
     L85:   void set_thread(JavaThread* thread) { _handshakee = thread; }
         nit - usually the 'setter' name matches the field name. So this 
could/should
         be called 'set_handshakee()'.

     L103:   Thread* get_active_handshaker() const { return 
_active_handshaker; }
         nit - usually 'get_' is omitted from the 'getter'. So this 
could/should
         be called 'active_handshaker()'.

src/hotspot/share/runtime/handshake.cpp
     L56:     assert(val >= 0, "_pending_threads cannot be negative");
         Perhaps:
              assert(val >= 0, "_pending_threads=%d: cannot be 
negative", val);

         Gives a little better debug info.

     L67:     assert(_pending_threads == 0, "Must be zero");
         Perhaps:
              assert(_pending_threads == 0, "Must be zero: %d", 
_pending_threads);

         Gives a little better debug info.

     L402:   if ( (is_direct && op != _operation_direct)) {
         nit - please delete extra space after first '(' and the
         inner '(' and ')' are not needed.

src/hotspot/share/runtime/threadSMR.hpp
     No comments.

src/hotspot/share/runtime/thread.hpp
     L1359:   Thread* get_active_handshaker() const {
         nit - If you rename 'get_active_handshaker()' to 
'active_handshaker()' as
         mentioned above, you should also rename this one.

src/hotspot/share/runtime/thread.cpp
     L4472:   // We must check there are no active references to this thread
         grammar - s/check there/check that there/

     L4476:   if 
(!ThreadsSMRSupport::is_a_protected_JavaThread((JavaThread *) thread)) {
         nit - The local 'thread' is a 'JavaThread*' so this cast should 
not be needed.

     L4479-82 - Thanks for the good comment about _thread_key in TLS.

src/hotspot/share/runtime/safepointMechanism.inline.hp
     No comments.

src/hotspot/share/runtime/safepoint.cppp
     No comments.

src/hotspot/share/runtime/biasedLocking.cpp
     No comments.

src/hotspot/share/runtime/mutexLocker.cpp
     L191:   if (Thread::current() == thread->get_active_handshaker()) 
return;
         nit - If you rename 'get_active_handshaker()' to 
'active_handshaker()' as
         mentioned above, you should also rename this one.

test/hotspot/jtreg/runtime/handshake/HandshakeDirectTest.java
     L26:  * @test HandshakeDirectTest
         nit - there should there be an "@bug 8240918" after this line.

     L64:                 if (handshakee == me) {
         Please add a comment after this line:
                              // Pick another thread instead of me.

     L63:                 int handshakee = 
ThreadLocalRandom.current().nextInt(0, WORKING_THREADS-1);
     L111:                     int i = 
ThreadLocalRandom.current().nextInt(0, WORKING_THREADS-1);
         nit - please add spaces around '-'.

     L107:         Thread suspendResumeThread = new Thread() {
         So you don't do anything to shutdown the suspend-resume thread
         which will stress the use of suspend/resume at workingThreads
         exit time. I like it and I hope that's intentional.


Thumbs up! My comments above are nits so it is your call whether to address
them or not. I do not need to see a new webrev.

Dan



On 4/2/20 11:11 AM, Patricio Chilano wrote:
> Hi David,
>
> On 4/2/20 4:45 AM, David Holmes wrote:
>> Hi Patricio,
>>
>> On 2/04/2020 3:45 am, Patricio Chilano wrote:
>>> Hi all,
>>>
>>> Please review this redo for 8230594. Since the reason behind the 
>>> Windows timeouts turned out to be 8240902, this patch is almost 
>>> identical to the reverse of the backout modulo the rebase.
>>
>> Glad to see this come back! Overall changes still look good - naturally.
>>
>>> There is only one small fix for an issue not identified in the 
>>> original patch which I explained in the comments. To avoid this 
>>> possible issue of deleting a JavaThread that called 
>>> Thread::destroy_vm() while there are handshakers blocked in the 
>>> handshake_turn_sem semaphore, I added a check to verify the 
>>> JavaThread in question is not protected by some ThreadsList 
>>> reference before attempting the delete it. In case it is protected, 
>>> we need to at least clear the value for _thread_key to avoid 
>>> possible infinite loops at thread exit (this can happen in Solaris).
>>
>> Interesting problem. At some point during the shutdown sequence the 
>> thread needs to be able to execute "process any pending handshake and 
>> mark this thread as no longer handshake-capable". But it is unclear 
>> how to achieve that at this time so I'm okay with deferring this to a 
>> future RFE. Even though the VM has "terminated" this may be a custom 
>> launcher, or a hosted VM, and the main process is not necessarily 
>> going away, so it would be preferable to be able to delete the thread 
>> and free all its resources.
> Ok, deferring this to a future RFE sounds good. I  actually tried to 
> think how to safely delete the thread even in these cases but I can't 
> find a clean way without complicating too much the code. Since this is 
> a special case and only when the VM has already terminated I thought 
> this was the best approach. I see your point about the hosted VM case 
> though. But also as Coleen pointed out we are not freeing a lot of 
> other threads too so I don't think not freeing one extra thread will 
> hurt. Just to highlight that, before the main thread exits I see still 
> alive, not counting Compiler and GC threads, Finalizer, Reference 
> Handler, Signal Dispatch, Service Thread, Sweeper thread, 
> Common-Cleaner and Notification Thread. And although I don't see the 
> VMThread we are not actually deleting it. This is the comment at the 
> end of VMThread::run():
>
> // We are now racing with the VM termination being carried out in
> // another thread, so we don't "delete this". Numerous threads don't
> // get deleted when the VM terminates
>
> I'm not saying we shouldn't try to delete this thread because we are 
> not doing it with other threads though. But I think in this special 
> case keeping the code simpler is probably worth more than complicating 
> the code to free one extra thread.
>> One nit: in the ThreadSMR code this looks really odd:
>>
>>  static bool is_a_protected_JavaThread_with_lock(JavaThread *thread, 
>> bool skiplock = false);
>>
>> A "with lock" function that might not lock? I suggest just making 
>> is_a_protected_JavaThread public and calling it directly.
> Changed. Originally I didn't want to declare 
> is_a_protected_JavaThread() public to make it clear that 
> is_a_protected_JavaThread_with_lock() is the one that should be always 
> called. But there is an assertion that we either own the Threads_lock 
> or we are at a safepoint, so if somebody tries to call this in an 
> illegal scenario it should assert.
>
> Here is v2:
>
> Full: http://cr.openjdk.java.net/~pchilanomate/8240918/v2/webrev/
> Inc: http://cr.openjdk.java.net/~pchilanomate/8240918/v2/inc/webrev/
>
> Thanks for looking at this David!
>
> Patricio
>> Thanks,
>> David
>> -----
>>
>>> The only other change is that I removed the check for local polls in 
>>> Handshake::execute_direct() since after the backout, thread-local 
>>> handshakes was implemented for arm32 which was the only remaining 
>>> platform.
>>>
>>> I tested it several times in mach5 tiers 1-6 and once in t7 and saw 
>>> no failures.
>>>
>>> Bug:
>>>     https://bugs.openjdk.java.net/browse/JDK-8240918
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~pchilanomate/8240918/v1/webrev/
>>>
>>>
>>> Thanks,
>>> Patricio
>



More information about the hotspot-runtime-dev mailing list