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

Patricio Chilano patricio.chilano.mateo at oracle.com
Wed Jul 15 20:50:20 UTC 2020


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
>



More information about the hotspot-runtime-dev mailing list