RFR 8251118: BiasedLocking::preserve_marks should not have a HandleMark
David Holmes
david.holmes at oracle.com
Wed Aug 12 00:36:01 UTC 2020
+1 on Coleen's comment about still including handles.hpp
+1 on changes looking good.
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
>>>>>>
>>>>>
>>>>
>>>
>>
>
More information about the hotspot-runtime-dev
mailing list