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

[Daniel D. Daugherty]

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