RFR 8232733: Remove need to grab Threads_lock while processing handshakes

[David Holmes]

On 12/12/2019 8:06 am, Patricio Chilano wrote:
> Hi David,
> 
> On 12/10/19 5:01 AM, David Holmes wrote:
>> Hi Patricio,
>>
>> On 10/12/2019 9:12 am, Patricio Chilano wrote:
>>> Hi David,
>>>
>>> On 12/9/19 12:12 AM, David Holmes wrote:
>>>> Hi Patricio,
>>>>
>>>> On 7/12/2019 5:40 am, Patricio Chilano wrote:
>>>>> Hi all,
>>>>>
>>>>> Please review the following patch that removes the requirement of 
>>>>> having to acquire the Threads_lock while processing handshakes. The 
>>>>> guarantee that the JavaThread being handshaked will not exit and 
>>>>> its memory be freed is already provided by using SMR ThreadsLists. 
>>>>> With respect to the comments that the Threads_lock is needed to 
>>>>> avoid a suspended JavaThread from being resumed, that will not be a 
>>>>> problem since the JavaThread will still make the proper transition 
>>>>> after being resumed and will block if needed for the handshake.
>>>>>
>>>>> Tested with mach5, tiers1-7.
>>>>>
>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8232733
>>>>> Webrev: http://cr.openjdk.java.net/~pchilanomate/8232733/v01/webrev/
>>>>
>>>> src/hotspot/share/runtime/handshake.cpp
>>>>
>>>>  151       // We need to re-think this with SMR ThreadsList.
>>>>  152       // There is an assumption in the code that the 
>>>> Threads_lock should be
>>>>  153       // locked during certain phases.
>>>>  154       {
>>>>  155         MutexLocker ml(Threads_lock);
>>>>
>>>> The way I read that comment is that the SMR code expects the 
>>>> Threads_lock to be held during certain phases of the handshake code. 
>>>> Is that not what it meant?
>>>>
>>>> Ditto for where we use the JavaThreadIteratorWithHandle. If we 
>>>> didn't need the Threads_lock while using the 
>>>> JavaThreadIteratorWithHandle why were we taking it? Were we just 
>>>> overly cautious/conservative?
>>> The use of the Threads_lock was there from the very first commit when 
>>> handshakes were introduced (8189941), before using 
>>> ThreadsListHandle/JavaThreadIteratorWithHandle. My understanding 
>>> based on having read the code and talking with Robbin is that the 
>>> Threads_lock was there for two reasons. One reason was so that the 
>>> JavaThread would not be freed while reading it (although looking at 
>>> that first commit it seems that between releasing the Threads_lock 
>>> after setting the handshake and acquiring it again before processing 
>>> it, the JavaThread could have already processed the handshake and 
>>> exited). The other reason was so that a suspended JavaThread would 
>>> not be resumed in the middle of the handshake.
>>> Now, the second commit to handshake.cpp (8167108) introduced the use 
>>> of SMR lists, removing the need to acquire the Threads_lock for 
>>> setting the handshake but not for processing it, since we still 
>>> needed it to prevent a suspended JavaThread from being resumed.
>>> Based on the last changes you made to suspend/resume code we know 
>>> that even if a JavaThread is resumed it will still transition and 
>>> stop for the handshake. So, more than conservative I would say it was 
>>> a requirement back then but not anymore. Having said that it would be 
>>> great If people that contributed to 8167108 can also review this 
>>> patch and validate my claims : )
>>
>> Thanks for the history! My concern is that 8167108 is the change that 
>> added the comment:
>>
>> +      // We need to re-think this with SMR ThreadsList.
>> +      // There is an assumption in the code that the Threads_lock 
>> should be
>> +      // locked during certain phases.
>>
>> so it would be good to here from those involved to know why that 
>> comment was added.
> Are you okay with this patch?

I'm a little concerned about what that comment actually was referring 
to. The reasoning people have given that the code should work without 
the Threads_lock sounds reasonable enough, but noone can explain that 
comment yet.

If this is for 15 then I'm okay seeing it pushed. but for 14 I'd like 
the author of the comment to clarify things.

Thanks,
David
-----

> 
> 
> Thanks,
> Patricio
>> Cheers,
>> David
>>
>>> Thanks,
>>>
>>> Patricio
>>>> Thanks,
>>>> David
>>>>
>>>>
>>>>> Thanks,
>>>>>
>>>>> Patricio
>>>
>