[15] RFR(S): 8246676: monitor list lock operations need more fencing and 8247280

[Daniel D. Daugherty]

Patricio,

Thanks for the review!

Dan



On 7/15/20 4:50 PM, Patricio Chilano wrote:
> Hi Dan,
>
> Changes look good to me. Thanks for the fixes.
>
> Patricio
> On 7/15/20 11:23 AM, Daniel D. Daugherty wrote:
>> On 7/15/20 2:46 AM, David Holmes wrote:
>>> Hi Dan,
>>>
>>> On 15/07/2020 12:30 pm, Daniel D. Daugherty wrote:
>>>> Greetings,
>>>>
>>>> These fixes are targeted for JDK15 and I would like to push both of 
>>>> these
>>>> fixes before the RDP2 cutoff on Thursday.
>>>>
>>>> I have a JDK15 fix ready for a couple of related ObjectMonitor bug 
>>>> fixes:
>>>>
>>>>      JDK-8246676 monitor list lock operations need more fencing
>>>>      https://bugs.openjdk.java.net/browse/JDK-8246676
>>>>
>>>>      JDK-8247280 more fencing needed in async deflation for non-TSO 
>>>> machines
>>>>      https://bugs.openjdk.java.net/browse/JDK-8247280
>>>>
>>>> The fix for JDK-8246676 has been through three rounds of preliminary
>>>> code review with David H.; Erik O. and Robbin participated in the 
>>>> first
>>>> preliminary code review round. Mostly comment changes or backouts of
>>>> code changes to return to the baseline code were done in the second 
>>>> and
>>>> third preliminary rounds.
>>>>
>>>> The fix for JDK-8247280 has been through two rounds of preliminary
>>>> code review with David H. The bug fix itself was suggested by Erik 
>>>> O. so
>>>> he's likely on-board with my implementation of the fix. :-)
>>>>
>>>> Many thanks to David H., Erik O. and Robbin for their many emails on
>>>> these topics and for reviewing the preliminary webrevs.
>>>>
>>>> Here are the two webrevs:
>>>>
>>>> http://cr.openjdk.java.net/~dcubed/8247280-webrev/0-for-jdk15/
>>>
>>> My only follow up here is the proper fix for using Atomics with 
>>> enums. The patch is below and I've tested it with tiers 1-3 (link 
>>> sent separately).
>>
>> Thanks! I appreciate the help with the metaprogramming stuff...
>>
>>
>>>
>>>> http://cr.openjdk.java.net/~dcubed/8246676-webrev/0-for-jdk15/
>>>
>>> Also looks good.
>>
>> Thanks!
>>
>>
>>>
>>>> The project is currently baselined on jdk-15+30 and has gone through
>>>> Mach5 Tier[1-3],4,5,6,7,8 testing with no regressions. I've also run
>>>> my inflation stress kit on Linux-X64 and macOSX without any 
>>>> regressions.
>>>>
>>>> Thanks, in advance, for any comments, questions or suggestions.
>>>>
>>>> Dan
>>>
>>> Thanks for working through all the details on this.
>>
>> Thank you for the many emails, discussions and reviews. This memory
>> consistency stuff is hard to get right.
>>
>>
>>>
>>> David
>>> -----
>>>
>>> diff -r 3bef86e53c51 src/hotspot/share/runtime/objectMonitor.hpp
>>> --- a/src/hotspot/share/runtime/objectMonitor.hpp
>>> +++ b/src/hotspot/share/runtime/objectMonitor.hpp
>>> @@ -27,6 +27,7 @@
>>>
>>>  #include "memory/allocation.hpp"
>>>  #include "memory/padded.hpp"
>>> +#include "metaprogramming/isRegisteredEnum.hpp"
>>>  #include "oops/markWord.hpp"
>>>  #include "runtime/os.hpp"
>>>  #include "runtime/park.hpp"
>>> @@ -372,4 +373,7 @@
>>>    void      install_displaced_markword_in_object(const oop obj);
>>>  };
>>>
>>> +// Register for atomic operations.
>>> +template<> struct IsRegisteredEnum<ObjectMonitor::AllocationState> 
>>> : public TrueType {};
>>> +
>>>  #endif // SHARE_RUNTIME_OBJECTMONITOR_HPP
>>> diff -r 3bef86e53c51 src/hotspot/share/runtime/objectMonitor.inline.hpp
>>> --- a/src/hotspot/share/runtime/objectMonitor.inline.hpp
>>> +++ b/src/hotspot/share/runtime/objectMonitor.inline.hpp
>>> @@ -196,7 +196,7 @@
>>>  }
>>>
>>>  inline void 
>>> ObjectMonitor::release_set_allocation_state(ObjectMonitor::AllocationState 
>>> s) {
>>> -  Atomic::release_store((int*)&_allocation_state, (int)s);
>>> +  Atomic::release_store(&_allocation_state, s);
>>>  }
>>>
>>>  inline void 
>>> ObjectMonitor::set_allocation_state(ObjectMonitor::AllocationState s) {
>>> @@ -208,7 +208,7 @@
>>>  }
>>>
>>>  inline ObjectMonitor::AllocationState 
>>> ObjectMonitor::allocation_state_acquire() const {
>>> -  return 
>>> (AllocationState)Atomic::load_acquire((int*)&_allocation_state);
>>> +  return Atomic::load_acquire(&_allocation_state);
>>>  }
>>>
>>>  inline bool ObjectMonitor::is_free() const {
>>
>> That ended up being much simpler than I imagined late last night... :-)
>>
>> Thanks for coding this up and for running it through a Mach5 Tier[1-3].
>> I very much appreciate it.
>>
>> Dan
>>
>