RFR(L) 8153224 Monitor deflation prolong safepoints

[Daniel D. Daugherty]

Just tracking to make sure I made all the changes...


On 4/8/19 9:04 PM, Daniel D. Daugherty wrote:
> On 4/5/19 4:59 PM, Karen Kinnear wrote:
>> 1. Could you add comments to markOop.hpp about
>> the use in the displaced_mark_word of is_marked to prevent any users 
>> of is_marked
>> here from needing to have that information saved/restored?
>
> I _think_ I know what you're looking for here... Perhaps this:
>
> src/hotspot/share/oops/markOop.hpp:
>   // ObjectMonitor::install_displaced_markword_in_object() uses
>   // is_marked() on ObjectMonitor::_header as part of the restoration
>   // protocol for an object's header. In this usage, the mark bit is
>   // only ever set (and cleared) on the ObjectMonitor::_header field.
>   bool is_marked()   const {
>     return (mask_bits(value(), lock_mask_in_place) == marked_value);
>   }

Done.


>
>> 2. In objectMonitor.hpp
>>   in is_busy you clarify the difference in use between _count (which 
>> I think you may be changing
>> to _contended) and _ref_count. Could you possibly also comment where 
>> you declare them?
>
> I'll do the rename of _count -> _contentions in a subtask of 8153224
> like the other cleanups of the monitor subsystem.

Done via 8222295.


> Here's the comment in question:
>
> src/hotspot/share/runtime/objectMonitor.hpp:
>   intptr_t is_busy() const {
>     // TODO-FIXME: merge _count and _waiters.
>     // TODO-FIXME: assert _owner == null implies _recursions = 0
>     // TODO-FIXME: assert _WaitSet != null implies _count > 0
>     // We do not include _ref_count in the is_busy() check because
>     // _ref_count is for indicating that the ObjectMonitor* is in
>     // use which is orthogonal to whether the ObjectMonitor itself
>     // is in use for a locking operation.
>     return 
> _count|_waiters|intptr_t(_owner)|intptr_t(_cxq)|intptr_t(_EntryList);
>   }
>
> I don't think this comment clarifies _count vs. _ref_count.
> I added the last four lines of the comment and their purpose is to
> describe why _ref_count isn't used by is_busy(). The TODO-FIXME lines
> need to be revisited since (at least) the third one is wrong.
>
> Here's the existing comment for _ref_count:
>
>   volatile jint _ref_count;         // ref count for ObjectMonitor*
>
> Here's the existing comment for _count:
>
>   volatile jint  _count;            // reference count to prevent 
> reclamation/deflation
>                                     // at stop-the-world time. See 
> ObjectSynchronizer::deflate_monitor().
>                                     // _count is approximately 
> |_WaitSet| + |_EntryList|
>
> And here's what I proposed to change it to in my reply to your design
> review notes:
>
>   volatile jint  _contentions;      // Number of active contentions in 
> enter(). It is used by is_busy()
>                                     // along with other fields to 
> determine if an ObjectMonitor can be
>                                     // deflated. See 
> ObjectSynchronizer::deflate_monitor().

Done via 8222295.


> I think we're good here with the proposed change of comment (and the
> rename) for the _contentions field along with existing comment for
> _ref_count and the existing comment for is_busy(). I may delete the
> third TODO-FIXME line as part of the next cleanup.

I deleted the first and the third TODO-FIXME lines. We're never
going to merge _count and _waiters; we have APIs that need those
returned independently of each other.

Done via 8222295.


>
>
>> 3. clear_using_JT: would it make sense to have an assertion that 
>>  _owner is either null or DEFLATER_MARKER?
>
> We could add something like:
>
>   assert(_owner == NULL ||
>          (AsyncDeflateIdleMonitors && _owner == DEFLATER_MARKER),
>          "Fatal logic error in ObjectMonitor owner!");
>
> and that will catch any races in async monitor deflation where the
> _owner field is set to a monitor owner value (stack addr or thread*).
> For monitor deflation at a safepoint, the non-NULL _owner field is
> caught in clear() (which calls clear_using_JT()).

Done. For now I've added:

   if (AsyncDeflateIdleMonitors) {
     guarantee(_owner == NULL || _owner == DEFLATER_MARKER,
               "Fatal logic error in ObjectMonitor owner!: owner="
               INTPTR_FORMAT, p2i(_owner));
     guarantee(_contentions <= 0,
               "Fatal logic error in ObjectMonitor contentions!: 
contentions=%d",
               _contentions);
   }


>
>> 4. In EnterI: if _owner == DEFLATER_MARKER there are guarantees that 
>> 0 < _count
>> with comments that caller ensured _count <= 0
>> In ReenterI: guarantee 0 <= _count, with comment not _count < 0
>> — Am I missing something subtle here or should they be the same 
>> guarantees?
>
> Here's the code in question:
>
> src/hotspot/share/runtime/objectMonitor.cpp:
>
> void ObjectMonitor::EnterI(TRAPS) {
> <snip>
>   if (_owner == DEFLATER_MARKER) {
>     guarantee(0 < _count, "_owner == DEFLATER_MARKER && _count <= 0 
> should have been handled by the caller");
>     // Deflater thread tried to lock this monitor, but it failed to 
> make _count negative and gave up.
>
> void ObjectMonitor::ReenterI(Thread * Self, ObjectWaiter * SelfNode) {
> <snip>
>     if (_owner == DEFLATER_MARKER) {
>       guarantee(0 <= _count, "Impossible: _owner == DEFLATER_MARKER && 
> _count < 0, monitor must not be owned by deflater thread here");
>
>
> Reading these two guarantee() calls always throws me off stride
> because I would have written them like this:
>
>     guarantee(_count > 0, "_owner == DEFLATER_MARKER && _count <= 0 
> should have been handled by the caller");
>
> and
>
>       guarantee(_count >= 0, "Impossible: _owner == DEFLATER_MARKER && 
> _count < 0, monitor must not be owned by deflater thread here");
>
> When rewritten like the above, you have:
>
>     "_count > 0" ... _count <= 0
>
> and:
>
>     "_count >= 0" ... "_count < 0"
>
> which is easier for my brain to read... okay... enough sidebar...
>
> Short answer: No the guarantees should not be the same.
>
> Longer answer: EnterI() is called by enter() after enter() has
> incremented the _count field to indicate the contended state of
> things. So in EnterI(), "_count > 0" is the right check.
> ReenterI() is called after wait() has returned (notified or
> timedout), and the _count field is not used on reentry ops so
> "_count >= 0" is the right check.
>
> I'm going to tweak the ObjectMonitor::EnterI() code like this (yes,
> there are two places in EnterI() that do this):
>
>     L501:   if (_owner == DEFLATER_MARKER) {
>               // The deflation protocol finished the first part 
> (setting _owner),
>               // but it failed the second part (making _count 
> negative) and bailed.
>               // Because we're called from enter() we have at least 
> one contention.
>               guarantee(count > 0, "_owner == DEFLATER_MARKER && 
> _count <= 0 should have been handled by the caller");
>     L504:     // Try to acquire monitor.
>     L505:     if (Atomic::cmpxchg(Self, &_owner, DEFLATER_MARKER) == 
> DEFLATER_MARKER) {

Done.


>     L629:     if (_owner == DEFLATER_MARKER) {
>                 // The deflation protocol finished the first part 
> (setting _owner),
>                 // but it failed the second part (making _count 
> negative) and bailed.
>                 // Because we're called from enter() we have at least 
> one contention.
>                 guarantee(count> 0 , "_owner == DEFLATER_MARKER && 
> _count <= 0 should have been handled by the caller");
>     L632:       if (Atomic::cmpxchg(Self, &_owner, DEFLATER_MARKER) == 
> DEFLATER_MARKER) {

Done.


> And I'm going to tweak the ReenterI() code like this:
>
>     L759:     if (_owner == DEFLATER_MARKER) {
>                 // The deflation protocol finished the first part 
> (setting _owner),
>                 // but it will observe _waiters != 0 and will bail 
> out. Because we're
>                 // called from wait() we may or may not have any 
> contentions.
>                 guarantee(count >= 0, "Impossible: _owner == 
> DEFLATER_MARKER && _count < 0 should have been handled by the caller");
>     L761:       if (Atomic::cmpxchg(Self, &_owner, DEFLATER_MARKER) == 
> DEFLATER_MARKER) {

Done.


>
>> 5. I could use a little help with allocation state transitions,
>> e.g. in deflate_monitor_list_using_JT
>>   you see is_new with object set so you mark it as old so next 
>> deflation will check it
>
> Here's the code in question:
>
> src/hotspot/share/runtime/synchronizer.cpp:
>
> int ObjectSynchronizer::deflate_monitor_list_using_JT(ObjectMonitor** 
> listHeadp,
> ObjectMonitor** freeHeadp,
> ObjectMonitor** freeTailp,
> ObjectMonitor** savedMidInUsep) {
> <snip>
>     // Only try to deflate if there is an associated Java object and if
>     // mid is old (is not newly allocated and is not newly freed).
>     if (mid->object() != NULL && mid->is_old() &&
>         deflate_monitor_using_JT(mid, freeHeadp, freeTailp)) {
>       // Deflation succeeded so update the in-use list.
> <snip>
>     } else {
>       // mid is considered in-use if it does not have an associated
>       // Java object or mid is not old or deflation did not succeed.
>       // A mid->is_new() node can be seen here when it is freshly returned
>       // by omAlloc() (and skips the deflation code path).
>       // A mid->is_old() node can be seen here when deflation failed.
>       // A mid->is_free() node can be seen here when a fresh node from
>       // omAlloc() is released by omRelease() due to losing the race
>       // in inflate().
>
>       if (mid->object() != NULL && mid->is_new()) {
>         // mid has an associated Java object and has now been seen
>         // as newly allocated so mark it as "old".
>         mid->set_allocation_state(ObjectMonitor::Old);
>       }
>
>>   - why do you set it to old here rather than in inflate once we set 
>> values?
>
> Inflation is used in quite a few places. If we marked the
> ObjectMonitor as "Old" in inflate(), then that would make the
> ObjectMonitor available for deflation by deflate_monitor_using_JT()
> earlier:
>
> src/hotspot/share/runtime/synchronizer.cpp:
>> bool ObjectSynchronizer::deflate_monitor_using_JT(ObjectMonitor* mid,
>> ObjectMonitor** freeHeadp,
>> ObjectMonitor** freeTailp) {
>>   assert(AsyncDeflateIdleMonitors, "sanity check");
>>   assert(Thread::current()->is_Java_thread(), "precondition");
>>   // A newly allocated ObjectMonitor should not be seen here so we
>>   // avoid an endless inflate/deflate cycle.
>>   assert(mid->is_old(), "precondition");
>
> So the idea behind only deflating ObjectMonitors that have reached
> allocation state "Old" is to prevent "an endless inflate/deflate cycle".
> Here's the relevant section from Carsten's JEP:
>
>> To avoid endless inflation / deflation cycles in the prototype, monitor
>> deflation is only attempted the second time a monitor is seen by the
>> thread marking monitors as deflatable: If the thread (the only thread
>> marking monitors as deflatable; might be service thread or some GC
>> related thread or even a dedicated thread) sees a monitor in state New,
>> then the thread marks the monitor as Old and moves on. So there is
>> little interaction between a thread inflating a lock to a monitor and
>> the deflating thread, the inflating thread just has to make sure the
>> monitor is marked New and this marker is published using appropriate
>> barriers.
>
> There isn't an explicit example in the JEP of what Carsten was thinking
> of with "an endless inflate/deflate cycle". I didn't try to think of
> such an example for the OpenJDK wiki either. I simple wrote:
>
>> ObjectMonitor has a new allocation_state field that supports three
>> states: 'Free', 'New', 'Old'. Async Monitor Deflation is only applied
>> to ObjectMonitors that have reached the 'Old' state. When the Async
>> Monitor Deflation code sees an ObjectMonitor in the 'New' state, it
>> is changed to the 'Old' state, but is not deflated. This prevents a
>> newly allocated ObjectMonitor from being immediately deflated which
>> could cause an inflation<->deflation oscillation.
>
> So let's think about what might happen if an ObjectMonitor is marked
> as "Old" in inflate(). Here's an example use of inflate() in the
> "slow enter" code path:
>
> src/hotspot/share/runtime/synchronizer.cpp:
> > void ObjectSynchronizer::slow_enter(Handle obj, BasicLock* lock, 
> TRAPS) {
> <snip>
> base<    inflate(THREAD, obj(), 
> inflate_cause_monitor_enter)->enter(THREAD);
>
> new>     ObjectMonitorHandle omh;
> new>     inflate(&omh, THREAD, obj(), inflate_cause_monitor_enter);
> new>     do_loop = !omh.om_ptr()->enter(THREAD);
>
> In the "base" code, we took the return from inflate() and used it to call
> ObjectMonitor::enter(). If we never changed that bit of code and inflate()
> marked the ObjectMonitor as "Old", then deflate_monitor_using_JT() could
> async deflate the ObjectMonitor while we were trying to call enter() on
> it... Boom! So we might think that holding off marking an ObjectMonitor
> as "Old" can save us... and it can, but not in all cases... :-(
>
> It is entirely possible that our call to slow_enter() is made on an
> ObjectMonitor that's already marked "Old". In that case, our thread
> (T-enter) calls inflate() which returns the existing ObjectMonitor*
> and we use it to call enter(). If the thread (T-deflate) calling
> deflate_monitor_using_JT() does its magic before T-enter sets the
> owner field or the count field... Boom!
>
> The previous paragraph is exactly what motivated the _ref_count field,
> the ObjectMonitorHandle helper, and adding an ObjectMonitorHandle*
> parameter to inflate(). inflate() calls ObjectMonitorHandle::save_om_ptr()
> which increments the ObjectMonitor's ref_count and then checks for async
> deflation protocol collisions. If there's a collision, then save_om_ptr()
> returns false and the caller (inflate() in this case) has to retry. When
> inflate() returns, the ObjectMonitor in the ObjectMonitorHandle cannot
> be deflated and is safe until the ObjectMonitorHandle is destroyed.
>
> So by changing T-enter to use an ObjectMonitorHandle, T-deflate cannot
> deflate the ObjectMonitor in the window after inflate() returns and
> before T-enter sets the owner field or increments the count field. But
> you know all that already!
>
> So let's bring this back to having inflate() mark the ObjectMonitor as
> "Old"... Since inflate() returns an ObjectMonitor with the ref_count > 0,
> it doesn't matter if the ObjectMonitor is marked as "Old" in inflate().
> T-deflate cannot deflate it due to ref_count > 0.

I moved the marking as "Old" into inflate() and deleted the block in
deflate_monitor_list_using_JT() that converted "New" into "Old" (when
there was an object ref).

This means that inflated ObjectMonitors will be eligible for deflation
as soon as the ref_count for the ObjectMonitor* does to zero in the
inflate() caller (when ObjectMonitorHandle is destroyed).


>
>> 6. Could you get rid of the new goto’s?
>
> I believe there is only one left from Carsten's prototype:
>
> src/hotspot/share/runtime/synchronizer.cpp:
>
>> intptr_t ObjectSynchronizer::FastHashCode(Thread * Self, oop obj) {
> <snip>
>>   } else if (mark->has_monitor()) {
>>     ObjectMonitorHandle omh;
>>     if (!omh.save_om_ptr(obj, mark)) {
>>       // Lost a race with async deflation so try again.
>>       assert(AsyncDeflateIdleMonitors, "sanity check");
>>       goto Retry;
>>     }
>
> I can change FastHashCode() to use the same "while (do_loop)" as the
> other code that needs to do retries...

Done. Changed it to a "while (true)" loop.


>
>>
>> 8. There is an old comment in FastHashCode
>> that
>>  // WARNING:
>>     //   The displaced header is strictly immutable.
>>     // It can NOT be changed in ANY cases.
>>
>> I presume that only applies to the displaced header for a stack lock 
>> - could you
>> possibly update that while you are in the code?
>
> Here's the whole comment:
>
>>     // WARNING:
>>     //   The displaced header is strictly immutable.
>>     // It can NOT be changed in ANY cases. So we have
>>     // to inflate the header into heavyweight monitor
>>     // even the current thread owns the lock. The reason
>>     // is the BasicLock (stack slot) will be asynchronously
>>     // read by other threads during the inflate() function.
>>     // Any change to stack may not propagate to other threads
>>     // correctly.
>
> That comment applies the displaced header that's in the BasicLock
> on the thread's stack and it definitely needs some cleaning up
> independent of the Async Monitor Deflation project.

Done via 8222295.


>
>> Also in FastHashCode
>>       // The only update to the header in the monitor (outside GC)
>>   823       // is install the hash code. If someone add new usage of
>>   824       // displaced header, please update this code
>> Can you update that comment as well? I know you’ve already updated 
>> the code logic.
>
> I'll revisit that comment as well. I believe Carsten updated it in
> his prototype, but when I backed out that change when I simplified
> the hashcode stuff due to ObjectMonitorHandles/ref_count.

Updated the comment in the baseline. I've also updated "hash code" to
"hashcode" and "hash_code" to "hashcode" so we have just one form.
Done via 8222295:

         // The only update to the ObjectMonitor's header/dmw field
         // is to merge in the hash code. If someone adds a new usage
         // of the header/dmw field, please update this code.

Added the following paragraph for Async Monitor Deflation:

         // ObjectMonitor::install_displaced_markword_in_object()
         // does mark the header/dmw field as part of async monitor
         // deflation, but that protocol cannot happen now due to
         // the ObjectMonitorHandle above.

The existing assert()'s will catch any errant marks:

         assert(test->is_neutral(), "invariant: header=" INTPTR_FORMAT, 
p2i((address)test));


Okay, I believe I've made all the changes for this set of comments
and replies...

Dan