RFR 8232733: Remove need to grab Threads_lock while processing handshakes

[David Holmes]

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.

Cheers,
David

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