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

[Thomas Schatzl]

Hi,

 v2 looks good to me.

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
>>>>
>>>
>>
>