RFR 8232733: Remove need to grab Threads_lock while processing handshakes

[Patricio Chilano]

Hi David,

On 12/12/19 12:23 AM, David Holmes wrote:
>
>
> 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.
Ok, pushed to 15. Thanks for reviewing this David!

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