RFR 8230594: Allow direct handshakes without VMThread intervention

[David Holmes]

Hi Dan,

On 17/01/2020 11:57 pm, Daniel D. Daugherty wrote:
> On 1/16/20 6:06 PM, David Holmes wrote:
>> On 17/01/2020 8:54 am, Daniel D. Daugherty wrote:
>>> On 1/16/20 5:40 PM, David Holmes wrote:
>>>> Hi Dan,
>>>>
>>>> Can I pick up on one suggestion you made:
>>>>
>>>>     L341:   assert(Thread::current() == _thread, "should call from 
>>>> thread");
>>>>         This should probably be JavaThread::current().
>>>>
>>>> If we get the wrong thread such that the assert fails, and that 
>>>> thread is not even a JavaThread, then we will get an assertion 
>>>> failure inside JavaThread::current() instead of failing the L341 
>>>> assertion.
>>>
>>> No problem with correcting my comment. You are right about where we'll
>>> fail when JavaThread::current() is called by a non-JavaThread. I don't
>>> see that as much of a problem since it will make it clear that
>>> JavaThread::current() was called unexpectedly by a non-JavaThread and
>>> the call trace will lead to the original assertion. YMMV...
>>
>> My point is simply that there is nothing wrong with the original code
> 
> Agreed. The original code works fine for the purpose of the assertion.
> 
> 
>> and that changing it doesn't improve anything
> 
> I think making it clear that we expect a JavaThread here is an improvement,
> but I suspect that's where we disagree. :-)
> 
> 
>> but instead can lead to a different assertion failing
> 
> I already agreed that this is the case... :-)
> 
> 
>> - which seems like a step in the wrong direction to me even if we can 
>> still recover from the misstep.
> 
> Maybe. Depends on whether you think that making it clear that we expect
> a JavaThread here is a "good thing" or not.
> 
> 
>>
>>>> So I suggest that Thread::current() is always the best method to use 
>>>> in an assertion involving thread identity.
>>>
>>> I might be a bit tired/fried at this point, but what do you
>>> think is the use case for JavaThread::current()?
>>
>> JavaThread::current() is for getting the current thread when you know 
>> it has to be a JavaThread (and the assert inside that checks that), so 
>> that we don't litter the code with casts and/or checks of 
>> Thread::current()->is_Java_thread(). But you know that ... :)
> 
> Ummm... this part of what you said:
> 
>  > JavaThread::current() is for getting the current thread when you know 
> it has to be a JavaThread
> 
> is exactly why I suggested changing:
> 
>    assert(Thread::current() == _thread, "should call from thread");
> 
> to:
> 
>    assert(JavaThread::current() == _thread, "should call from thread");
> 
> At the location of that assertion, the calling thread must be a JavaThread
> so using JavaThread::current() makes more sense to me and that certainly
> appears to match what you said in this comment...
> 
> However, the focus of the assertion is that the JavaThread that is
> calling HandshakeState::process_self_inner() must match the cached
> "JavaThread *_thread" field in HandshakeState. If process_self_inner()
> happens to be called by a non-JavaThread or if it is called by the
> wrong JavaThread, then we want to fail this assertion:
> 
>    assert(Thread::current() == _thread, "should call from thread");
> 
> in both cases. So your point, if I understand correctly, is that the
> non-JavaThreadness is a secondary consideration. The primary consideration
> is that process_self_inner() was called from the wrong thread.

Exactly. If we have the wrong thread it doesn't matter if it is a 
JavaThread or not.

> Maybe this tweak will make things more clear:
> 
>    assert(Thread::current() == _thread, "called by the wrong thread");

Ah now I see the typo in the existing text:

  341   assert(Thread::current() == _thread, "should call from thread");

should say

  341   assert(Thread::current() == _thread, "should call from _thread");

Cheers,
David

> 
> Hopefully, I now understand what you're trying to say here... :-)
> 
> Dan
>