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

[Patricio Chilano]

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