RFR(L) 8153224 Monitor deflation prolong safepoints (CR7/v2.07/10-for-jdk14)

Daniel D. Daugherty daniel.daugherty at oracle.com
Tue Nov 5 15:36:37 UTC 2019


Hi David,

On 11/5/19 12:31 AM, David Holmes wrote:
> Hi Dan,
>
> On 5/11/2019 11:31 am, Daniel D. Daugherty wrote:
>> Hi David,
>>
>> Thanks for continuing to provide feedback on the Async Monitor Deflation
>> project! I appreciate your reviews very much...
>>
>> Responses embedded below (as usual)...
>
> Ditto. :)

And again...


>
>>
>> On 11/4/19 1:28 AM, David Holmes wrote:
>>> Hi Dan,
>>>
>>> A few follow ups to your responses, with trimming ...
>>>
>>> On 30/10/2019 6:20 am, Daniel D. Daugherty wrote:
>>>> On 10/24/19 7:00 AM, David Holmes wrote:
>>>>>  122 // Set _owner field to new_value; current value must match 
>>>>> old_value.
>>>>>  123 inline void ObjectMonitor::set_owner_from(void* new_value, 
>>>>> void* old_value) {
>>>>>  124   void* prev = Atomic::cmpxchg(new_value, &_owner, old_value);
>>>>>  125   ADIM_guarantee(prev == old_value, "unexpected prev owner=" 
>>>>> INTPTR_FORMAT
>>>>>
>>>>> The use of cmpxchg seems a little strange here if you are 
>>>>> asserting that when this is called _owner must equal old_value. 
>>>>> That means you don't expect any race and if there is no race with 
>>>>> another thread writing to _owner then you don't need the cmpxchg. 
>>>>> A normal:
>>>>>
>>>>> if (_owner == old_value) {
>>>>>    Atomic::store(&_owner, new_value);
>>>>>    log(...);
>>>>> } else {
>>>>>    guarantee(false, " unexpected old owner ...");
>>>>> }
>>>>
>>>> The two parameter version of set_owner_from() is only called from 
>>>> three
>>>> places and we'll cover two of them here:
>>>>
>>>> src/hotspot/share/runtime/objectMonitor.cpp:
>>>>
>>>> 1041     if (AsyncDeflateIdleMonitors) {
>>>> 1042       set_owner_from(NULL, Self);
>>>> 1043     } else {
>>>> 1044       OrderAccess::release_store(&_owner, (void*)NULL); // 
>>>> drop the lock
>>>> 1045       OrderAccess::storeload(); // See if we need to wake a 
>>>> successor
>>>> 1046     }
>>>>
>>>> and:
>>>>
>>>> 1221   if (AsyncDeflateIdleMonitors) {
>>>> 1222     set_owner_from(NULL, Self);
>>>> 1223   } else {
>>>> 1224     OrderAccess::release_store(&_owner, (void*)NULL);
>>>> 1225     OrderAccess::fence(); // ST _owner vs LD in unpark()
>>>> 1226   }
>>>>
>>>> So I've replaced the existing {release_store(), storeload()} combo 
>>>> for one
>>>> call site and the existing {release_store(), fence()} combo for the 
>>>> other
>>>> call site with a cmpxchg(). I chose cmpxchg() for these reasons:
>>>>
>>>> 1) I wanted the same memory sync behavior at both call sites.
>>>> 2) I wanted similar/same memory sync behavior as the original
>>>>     code at those call sites.
>>>
>>> Why? The memory sync requirements for non-async deflation may be 
>>> completely different to those required for async-delfation (given 
>>> all the other bits if the protocol).
>>
>> Good point!
>>
>> For context, the first code block above (L1041-6) is in 
>> ObjectMonitor::exit()
>> and the second code block above (L1221-6) is in 
>> ObjectMonitor::ExitEpilog()
>> which is called from two different places by ObjectMonitor::exit(). 
>> In both
>> cases, we are setting the _owner field to NULL which will potentially 
>> make
>> the ObjectMonitor async deflatible (depending on ref_count).
>>
>> For async deflation, I want the full fence semantics after setting the
>> _owner field to NULL in both locations:
>>
>> src/hotspot/share/runtime/orderAccess.hpp:
>> //                       Constraint     x86          sparc 
>> TSO          ppc
>> // 
>> ---------------------------------------------------------------------------
>> // fence                 LoadStore  |   lock         membar 
>> #StoreLoad  sync
>> //                       StoreStore |   addl 0,(sp)
>> //                       LoadLoad   |
>> //                       StoreLoad
>> //
>> // release               LoadStore | lwsync
>> //                       StoreStore
>>
>> I don't want any loads or stores floating into or out of the critical 
>> region.
>>
>>
>> *** Side bar here ****
>>
>> I just noticed something with the original code:
>>
>> 1044       OrderAccess::release_store(&_owner, (void*)NULL); // drop 
>> the lock
>> 1045       OrderAccess::storeload();                        // See if 
>> we need to wake a successor
>>
>> For constraints, this gives us:
>>             {LoadStore | StoreStore}
>>             {StoreLoad}
>> at L1044-5. So the original code is just "missing" LoadLoad relative
>> to a full fence(). I'm not sure why this kind of load is allowed to
>> float into the critical region, but the code has been this way for a
>> very long time.
>
> You seem to overlooking the fact that your store appears between the 
> various memory barriers e.g.
>
>              {LoadStore | StoreStore}
>              ST _owner, 0
>              {StoreLoad}
>
> which establishes the effects of those barriers with respect to that 
> store. So loadload() would be superfluous as we've already ensured 
> that no loads can float above the store, due to the storeload barrier.

Ouch! And yes I did overlook the store. This is what I get for writing
a reply late in the day and not letting it sit for re-review the next
morning. My apologies for being in a hurry.


> A full fence is logically all 4 barriers in that it ensures all loads 
> and all stores remain on their respective sides of the fence - nothing 
> can cross it.
>
>> And for this original code:
>>
>> 1224     OrderAccess::release_store(&_owner, (void*)NULL);
>> 1225     OrderAccess::fence();                               // ST 
>> _owner vs LD in unpark()
>>
>> For constraints, this gives us:
>>           {LoadStore | StoreStore}
>>           {LoadStore | StoreStore | LoadLoad | StoreLoad}
>> at L1224-5. Again this code has been this way for a very long time.
>>
>> It seems to me that L1224-5 could be written like this:
>>
>> 1224     _owner = NULL;
>> 1225     OrderAccess::fence();                               // ST 
>> _owner vs LD in unpark()
>>
>> with a plain store on L1224. Is that correct?
>
> No, I don't believe so.

And neither do I in the morning light. :-)


> What we are also in danger of overlooking here is the presence of 
> memory synchronization instructions related to the semantics of a 
> synchronized code block in Java, and the presence of memory 
> synchronization instructions needed for the correct implementation of 
> the synchronization subsystem itself. Specifically given:
>
> OrderAccess::release_store(&_owner, (void*)NULL);
> OrderAccess::<some other sync op>
>
> The release store ensures that the releasing of the monitor cannot be 
> reordered with respect to any of the stores that occurred within the 
> synchronized block at the Java-level. (And it _might_ also ensure some 
> property of the sync implementation.) While "some other op" is 
> typically needed only because of the way we implement the 
> synchronization subsystem - as per the comments e.g.
>
> storeload(); // See if we need to wake a successor
>
> we don't want to load a potential successor before we set _owner to 
> NULL else we might read the wrong value.

Agreed.


>
>> *** End side bar ***
>>
>>
>>>
>>>> 3) I wanted the return value from cmpxchg() for my state machine
>>>>     sanity check.
>>>
>>> I'm somewhat dubious about using cmpxchg just for the side-effect of 
>>> getting the existing value.
>>
>> But I'm not "using cmpxchg just for the side-effect of getting the 
>> existing value".
>>
>> That's the third thing on my list of three reasons. The most important
>> thing is I want the full fence that cmpcxhg() gives me. Above I said:
>>
>>  > 1) I wanted the same memory sync behavior at both call sites.
>>  > 2) I wanted similar/same memory sync behavior as the original
>>  >     code at those call sites.
>>
>> Using cmpxchg() gives me the full fence I want and that's similar to
>> this baseline code at this call site:
>
> Yes but the memory sync effects of cmpxchg are secondary to it primary 
> purpose: which is to provide an atomic compare-and-exchange in the 
> face of concurrent updates to a variable.

Agreed.


>
>> 1044       OrderAccess::release_store(&_owner, (void*)NULL); // drop 
>> the lock
>> 1045       OrderAccess::storeload();                        // See if 
>> we need to wake a successor
>>
>> I'm getting the LoadLoad that the baseline site doesn't have.
>
> And which it doesn't need.

And my brain returns to the more fundamental question of why do we have
OrderAccess::storeload() at L1045 and OrderAccess::fence() at L1225?
Both sites are trying to separate the release_store(&_owner, NULL) from
a subsequent load. In the first case:

1044       OrderAccess::release_store(&_owner, (void*)NULL); // drop the 
lock
1045       OrderAccess::storeload();                        // See if we 
need to wake a successor
<snip>
1047     if ((intptr_t(_EntryList)|intptr_t(_cxq)) == 0 || _succ != NULL) {

In the second case:

1224     OrderAccess::release_store(&_owner, (void*)NULL);
1225     OrderAccess::fence();                               // ST 
_owner vs LD in unpark()
<snip>
1229   Trigger->unpark();

The code has been this way for a very long time, but why?

Of course, this question about the baseline code is still a sidebar.
An interesting sidebar, but...


For Async Monitor Deflation, I think we need fence() at both locations
for proper interaction with the deflater thread.


>
>> The cmpxchg() gives me the same memory constaints as this baseline code
>> at this call site:
>>
>> 1224     OrderAccess::release_store(&_owner, (void*)NULL);
>> 1225     OrderAccess::fence();                               // ST 
>> _owner vs LD in unpark()
>>
>> Note: Actually, I don't have the extra {LoadStore | StoreStore} from
>> the release_store() that I mentioned in the side bar above...
>>
>> The last thing that I get is the existing value...
>>
>>
>> Okay, so I thought it was a pretty cool use of cmpxchg(), but I'm
>> obviously confusing code readers. So here's the v2.08 set_owner_from():
>>
>>   124 // Set _owner field to new_value; current value must match 
>> old_value.
>>   125 inline void ObjectMonitor::set_owner_from(void* new_value, 
>> void* old_value) {
>>   126   void* prev = Atomic::cmpxchg(new_value, &_owner, old_value);
>>   127   ADIM_guarantee(prev == old_value, "unexpected prev owner=" 
>> INTPTR_FORMAT
>>   128                  ", expected=" INTPTR_FORMAT, p2i(prev), 
>> p2i(old_value));
>>   129   log_trace(monitorinflation, owner)("set_owner_from(): mid=" 
>> INTPTR_FORMAT
>>   130                                      ", prev=" INTPTR_FORMAT ", 
>> new="
>>   131                                      INTPTR_FORMAT, p2i(this), 
>> p2i(prev),
>>   132                                      p2i(new_value));
>>   133 }
>>
>> I could change it like this:
>>
>>   124 // Set _owner field to new_value; current value must match 
>> old_value.
>>   125 inline void ObjectMonitor::set_owner_from(void* new_value, 
>> void* old_value) {
>>   126   void* prev = _owner;
>>   127   ADIM_guarantee(prev == old_value, "unexpected prev owner=" 
>> INTPTR_FORMAT
>>   128                  ", expected=" INTPTR_FORMAT, p2i(prev), 
>> p2i(old_value));
>>   129   _owner = new_value;
>>   130   OrderAccess::fence();
>>   131   log_trace(monitorinflation, owner)("set_owner_from(): mid=" 
>> INTPTR_FORMAT
>>   132                                      ", prev=" INTPTR_FORMAT ", 
>> new="
>>   133                                      INTPTR_FORMAT, p2i(this), 
>> p2i(prev),
>>   134                                      p2i(new_value));
>>   135 }
>>
>> It's two lines longer, but it should require less head scratching to
>> figure out what I'm trying to do. Would this be acceptable?
>
> As per previous discussion I think you still need a release_store of 
> _owner (at least in the case where you are releasing the monitor).

I've made a mistake with this encapsulation. I made it look like a
general setter of a new value. In reality, both callers specify
new_value == NULL so we don't actually need the new_value parameter.

I think it needs to be something like this:

   124 // Clear _owner field; current value must match old_value.
   125 inline void ObjectMonitor::clear_owner_from(void* old_value) {
   126   void* prev = _owner;
   127   ADIM_guarantee(prev == old_value, "unexpected prev owner=" 
INTPTR_FORMAT
   128                  ", expected=" INTPTR_FORMAT, p2i(prev), 
p2i(old_value));
   129   OrderAccess::release_store(&_owner, (void*)NULL);
   130   OrderAccess::fence();
   131   log_trace(monitorinflation, owner)("clear_owner_from(): mid=" 
INTPTR_FORMAT
   132                                      ", prev=" INTPTR_FORMAT, 
p2i(this), p2i(prev));
   133 }

Thanks for sticking with this part of the review.


> That's it on this thread. I still have to look at version 2.08 in full.

I look forward to your feedback!

Dan


>
> Thanks,
> David
> -----
>
>>
>>
>>>
>>>> I don't think that using 'Atomic::store(&_owner, new_value)' is the
>>>> right choice for these two call sites.
>>>
>>> If you don't actually need the cmpxchg to handle concurrent updates 
>>> to the _owner field, then a plain store (not an Atomic::store - that 
>>> was an error on my part) does not seem unreasonable; or if there are 
>>> still memory sync issues here, perhaps a release_store.
>>
>> So in the above proposed code I switched to a plain store followed by
>> a fence().
>>
>>
>>> If you use cmpxchg then anyone reading the code will assume there is 
>>> a concurrent update that you are guarding against.
>>
>> Yup. I concede the point that I'm obviously confusing the other
>> code readers... sorry about that...
>>
>>
>>>
>>>> The last two parameter set_owner_from() is talked about in the
>>>> next reply.
>>>>
>>>>
>>>>> Similarly for the old_value1/old_valuie2 version.
>>>>
>>>> The three parameter version of set_owner_from() is only called from 
>>>> one
>>>> place and the last two parameter version is called from the same 
>>>> place:
>>>>
>>>> src/hotspot/share/runtime/synchronizer.cpp:
>>>>
>>>> 1903       if (AsyncDeflateIdleMonitors) {
>>>> 1904         m->set_owner_from(mark.locker(), NULL, DEFLATER_MARKER);
>>>> 1905       } else {
>>>> 1906         m->set_owner_from(mark.locker(), NULL);
>>>> 1907       }
>>>>
>>>> The original code was:
>>>>
>>>> 1399       m->set_owner(mark.locker());
>>>>
>>>> The original set_owner() code was defined like this:
>>>>
>>>>    87 inline void ObjectMonitor::set_owner(void* owner) {
>>>>    88   _owner = owner;
>>>>    89 }
>>>>
>>>> So the original code didn't do any memory sync'ing at all and I've
>>>> changed that to a cmpxchg() on both code paths. That appears to be
>>>> overkill for that callsite...
>>>
>>> Again I'm not sure any memory sync requirements from the non-async 
>>> case should necessarily transfer over to the async case. Even if you 
>>> end up requiring similar memory sync the reasoning would be quite 
>>> different I would expect.
>>
>> In this case, both async deflation and safepoint based deflation are
>> happy with the same memory sync because the newly allocated 
>> ObjectMonitor
>> isn't published yet so it is not deflatible by either mechanism. Also 
>> the
>> act of publishing the ObjectMonitor* will take care of the memory sync.
>>
>>
>>>
>>>>
>>>> We're in ObjectSynchronizer::inflate(), in the "CASE: stack-locked"
>>>> section of the code. We've gotten our ObjectMonitor from om_alloc()
>>>> and are initializing a number of fields in the ObjectMonitor. The
>>>> ObjectMonitor is not published until we do:
>>>>
>>>> 1916       object->release_set_mark(markWord::encode(m));
>>>>
>>>> So we don't need the memory sync'ing features of the cmpxchg() for
>>>> either of the set_owner_from() calls and all that leaves is the
>>>> state machine sanity check.
>>>>
>>>> I really like the state machine sanity check on the owner field but
>>>> that's just because it came in handy when chasing the recent races.
>>>> It would be easy to change the three parameter version of
>>>> set_owner_from() to not do memory sync'ing, but still do the state
>>>> machine sanity check.
>>>>
>>>> Update: Changing the three parameter version of set_owner_from()
>>>> may impact the changes to owner_is_DEFLATER_MARKER() discussed
>>>> above. Sigh...
>>>> Update 2: Probably no impact because the three parameter version of
>>>> set_owner_from() is only used before the ObjectMonitor is published
>>>> and owner_is_DEFLATER_MARKER() is used after the ObjectMonitor has
>>>> appeared on an in-use list.
>>>>
>>>> However, the two parameter version of set_owner_from() needs its
>>>> memory sync'ing behavior for it's objectMonitor.cpp call sites so
>>>> this call site would need something different.
>>>>
>>>> I'm not sure which solution I'm going to pick yet, but I definitely
>>>> have to change something here since we don't need cmpxchg() at this
>>>> call site. More thought is required.
>>>
>>> I will look to see where this ended up.
>>
>> I'll wait to see if you can live with the v2.08 version. I hope so...
>>
>>
>>>
>>>>> src/hotspot/share/runtime/objectMonitor.cpp
>>>>>
>>>>>
>>>>>  267   if (AsyncDeflateIdleMonitors &&
>>>>>  268       try_set_owner_from(Self, DEFLATER_MARKER) == 
>>>>> DEFLATER_MARKER) {
>>>>
>>>> For more context, we are in:
>>>>
>>>>   241 void ObjectMonitor::enter(TRAPS) {
>>>>
>>>>
>>>>> I don't see why you need to call try_set_owner_from again here as 
>>>>> "cur" will already be DEFLATER_MARKER from the previous 
>>>>> try_set_owner.
>>>>
>>>> I assume the previous try_set_owner() call you mean is this one:
>>>>
>>>>   248   void* cur = try_set_owner_from(Self, NULL);
>>>>
>>>> This first try_set_owner() is for the most common case of no owner.
>>>>
>>>> The second try_set_owner() call is for a different condition than 
>>>> the first:
>>>>
>>>>   268       try_set_owner_from(Self, DEFLATER_MARKER) == 
>>>> DEFLATER_MARKER) {
>>>>
>>>> L248 is trying to change the _owner field from NULL -> 'Self'.
>>>> L268 is trying to change the _owner field from DEFLATER_MARKER to 
>>>> 'Self'.
>>>>
>>>> If the try_set_owner() call on L248 fails, 'cur' can be several 
>>>> possible
>>>> values:
>>>>
>>>>    - the calling thread (recursive enter is handled on L254-7)
>>>>    - other owning thread value (BasicLock* or Thread*)
>>>>    - DEFLATER_MARKER
>>>
>>> I'll give a caution okay to that explanation (the deficiency being 
>>> in my understanding, not your explaining :) ).
>>
>> Thanks. I'll take it!
>>
>>
>>>
>>>>> Further, I don't see how installing self as the _owner here is 
>>>>> valid and means you acquired the monitor, as the fact it was 
>>>>> DEFLATER_MARKER means it is still being deflated by another thread 
>>>>> doesn't it ???
>>>>
>>>> I guess the comment after L268 didn't work for you:
>>>>
>>>>   269     // The deflation protocol finished the first part 
>>>> (setting owner),
>>>>   270     // but it failed the second part (making ref_count 
>>>> negative) and
>>>>   271     // bailed. Or the ObjectMonitor was async deflated and 
>>>> reused.
>>>>
>>>> It means that the deflater thread was racing with this enter and
>>>> managed to set the owner field to DEFLATER_MARKER as the first step
>>>> in the deflation protocol. Our entering thread actually won the race
>>>> when it managed to set the ref_count to a positive value as part of
>>>> the ObjectMonitorHandle stuff done in the inflate() call that preceded
>>>> the enter() call. However, the deflater thread hasn't realized that it
>>>> lost the race yet and hasn't restored the owner field back to NULL.
>>>
>>> You're right the comment didn't work for me as it required me to be 
>>> holding too much of the protocol in my head. Makes more sense now.
>>
>> Good to hear!
>>
>>
>>>
>>> Thanks,
>>> David
>>> -----
>>
>> Thanks again for the thorough reviews!
>>
>> Dan
>>



More information about the hotspot-runtime-dev mailing list