RFR(L) 8153224 Monitor deflation prolong safepoints (CR7/v2.07/10-for-jdk14)
Daniel D. Daugherty
daniel.daugherty at oracle.com
Tue Nov 5 01:31:49 UTC 2019
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)...
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.
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?
*** 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:
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.
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?
>
>> 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