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

[Patricio Chilano]

Hi Thomas,

On 8/11/20 5:47 PM, Thomas Schatzl wrote:
> Hi,
>
>   v2 looks good to me.
Great, thanks for reviewing this!


Patricio
> Thanks for fixing this,
>    Thomas
>
> ----- Original Message -----
> From: patricio.chilano.mateo at oracle.com
> To: daniel.daugherty at oracle.com, hotspot-runtime-dev at openjdk.java.net
> Sent: Tuesday, August 11, 2020 10:45:28 PM GMT +01:00 Amsterdam / Berlin / Bern / Rome / Stockholm / Vienna
> Subject: Re: RFR 8251118: BiasedLocking::preserve_marks should not have a HandleMark
>
>
> 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.
>>
>>
>>>> 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/
>
> 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
>>>>>