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

[Patricio Chilano]

On 4/2/20 10:15 PM, David Holmes wrote:
> Hi Patricio,
>
> Updates look good - thanks.
Thanks David!  I'll send a v3 addressing Dan's and Martin's comments.


Patricio
> 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
>>