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

[David Holmes]

Hi Patricio,

Updates look good - thanks.

David

On 3/04/2020 1: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
>