RFR 8251118: BiasedLocking::preserve_marks should not have a HandleMark

[Patricio Chilano]

On 8/11/20 9:36 PM, David Holmes wrote:
> +1 on Coleen's comment about still including handles.hpp
>
> +1 on changes looking good.
Thanks for the review David!


Patricio
> Thanks,
> David
>
> On 12/08/2020 6:51 am, Coleen Phillimore wrote:
>>
>>
>> On 8/11/20 4:45 PM, Patricio Chilano wrote:
>>>
>>> On 8/11/20 5:34 PM, Daniel D. Daugherty wrote:
>>>> On 8/11/20 4:27 PM, Patricio Chilano wrote:
>>>>> Hi Dan,
>>>>>
>>>>> Thanks for looking at this.
>>>>>
>>>>> On 8/11/20 4:53 PM, Daniel D. Daugherty wrote:
>>>>>> On 8/11/20 2:11 PM, Patricio Chilano wrote:
>>>>>>> Hi all,
>>>>>>>
>>>>>>> Please review the following small fix which involves removing 
>>>>>>> the added HandleMark in BiasedLocking::preserve_marks():
>>>>>>>
>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8251118
>>>>>>> Webrev: http://cr.openjdk.java.net/~pchilanomate/8251118/v1/webrev/
>>>>>>
>>>>>> src/hotspot/share/runtime/biasedLocking.cpp
>>>>>>     No comments.
>>>>>>
>>>>>> As mentioned by Harold, adding a new test case to:
>>>>>>
>>>>>>     test/hotspot/jtreg/runtime/7158988/FieldMonitor.java
>>>>>>
>>>>>> that enables '-XX:+UseBiasedLocking' is a good idea.
>>>>> I suggested adding it to test TestFullGCALot.java instead because 
>>>>> it fails much more consistently.
>>>>
>>>> I'm good with that also.
>>
>> Me too, even better than FieldMonitor.java if it's more reproduceable.
>>
>>>>
>>>>
>>>>>
>>>>>> Also, Coleen added this note to your bug report:
>>>>>>
>>>>>>> Also in the patch for JDK-8249192 
>>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8249192>, vframe.hpp 
>>>>>>> shouldn't include
>>>>>>> handles.inline.hpp (only cpp and inline.hpp files can include
>>>>>>> inline.hpp). It doesn't look like it needs handles.inline.hpp,
>>>>>>> only handles.hpp. 
>>>>>>
>>>>>> Were you planning on resolving that left over in this bug?
>>>>> Not really. I can change it here if you are okay though.
>>>>
>>>> Your call on whether to fix it here or with a new bug. Seems
>>>> like fixing it here is less work... this bug is a follow-on to
>>>> the original and that comment is a follow-on to the original...
>>> Yeah, sounds good to me to remove the extra header in this change. 
>>> I'll check it builds fine on all platforms first.
>>> Below is v2 in case you wanted to see it:
>>>
>>> Full: http://cr.openjdk.java.net/~pchilanomate/8251118/v2/webrev/
>>> Inc: http://cr.openjdk.java.net/~pchilanomate/8251118/v2/inc/webrev/
>>
>> I think vframe.hpp should include handles.hpp because there are 
>> Handle types in it, just in case something changes and it's not 
>> transiently included anymore.
>>
>> Otherwise, everything looks good.  I don't need to see another webrev.
>>
>> thanks!
>> Coleen
>>>
>>> Thanks Dan!
>>>
>>> Patricio
>>>> Dan
>>>>
>>>>
>>>>>
>>>>> Patricio
>>>>>> Dan
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> I've inspected the callers of BiasedLocking::preserve_marks() 
>>>>>>> and they all have an assert that the current thread is the 
>>>>>>> VMThread. Since the VMThread creates a HandleMark object before 
>>>>>>> executing a VM operation the extra HandleMark added in 8249192 
>>>>>>> is not needed.
>>>>>>> I've run tiers1-3 in mach5 with -XX:+UseBiasedLocking and 
>>>>>>> without the patch I get several crashes in 
>>>>>>> BiasedLocking::restore_marks(). With the patch tests completed 
>>>>>>> successfully.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Patricio
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>