RFR(L) 8153224 Monitor deflation prolong safepoints

[Daniel D. Daugherty]

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


On 4/9/19 11:53 AM, Daniel D. Daugherty wrote:
> Hi Carsten,
>
> Thanks for responding to Karen's code review comments.
>
> Karen,
>
> I have a query for you down at the end of my reply...
>
> More below...
>
> On 4/5/19 11:01 PM, Carsten Varming wrote:
>> Dear Karen,
>>
>> Please see inline answers.
>>
>> On Fri, Apr 5, 2019 at 4:59 PM Karen Kinnear 
>> <karen.kinnear at oracle.com <mailto:karen.kinnear at oracle.com>> wrote:
>>
>>
>>     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?
>>
>>
>> In ::enter _count is incremented when the thread is trying to acquire 
>> the monitor and decremented after the monitor has been acquired. The 
>> 0 < _count assertion is between those two point in the code. A thread 
>> acquiring a monitor and then calling wait will increment _count and 
>> then decrement _count as part of acquiring the monitor, thus _count 
>> can be 0 by the time the thread calls wait and when ReenterI is called.
>
> I had a similar answer and I'm planning to tweak the comments
> and the guarantees a bit in the next round of code review (CR1);
> please see my reply to Karen's CR for the proposed changes.

Done.


>
>>
>>     9. install_displaced_markword_in_object
>>     What happens if the cas_set_mark fails?
>>     I get that today this handles the race with enter and
>>     deflate_monitor_using_JT. If we remove
>>     the call from enter, is the expectation that we’ve blocked all
>>     others who did not set is_marked themselves?
>>     If we remove the call from enter would it make sense to ensure
>>     that the cas_set_mark succeeds here?
>>
>>
>> I designed my original patch such that no thread would ever wait for 
>> the the deflating thread to finish deflating a monitor. If you remove 
>> install_displaced_markword_in_object from enter, then the entering 
>> thread can end up busy waiting by continuously reading the monitor 
>> pointer from the object mark word and then realizing that the monitor 
>> is being deflated and it should retry by going back to reading the 
>> object mark word. This bad behavior is completely avoided by calling 
>> install_displaced_markword_in_object.
>
> Here's the code in question:
>
> src/hotspot/share/runtime/objectMonitor.cpp:
>
>> bool ObjectMonitor::enter(TRAPS) {
> <snip>
>>   // Prevent deflation. See ObjectSynchronizer::deflate_monitor() and 
>> is_busy().
>>   // Ensure the object-monitor relationship remains stable while 
>> there's contention.
>>   const jint count = Atomic::add(1, &_count);
>>   if (count <= 0 && _owner == DEFLATER_MARKER) {
>>     // Async deflation in progress. Help deflater thread install
>>     // the mark word (in case deflater thread is slow).
>>     install_displaced_markword_in_object();
>>     Self->_Stalled = 0;
>>     return false;  // Caller should retry. Never mind about _count as 
>> this monitor has been deflated.
>>   }
>
> Our thread (T-enter) observes that the ObjectMonitor is being deflated
> by T-deflate, calls install_displaced_markword_in_object() and returns
> false to the caller which causes a retry.
>
> Restoring the header/dmw from the ObjectMonitor to the object's header
> here isn't needed for correctness so it could be dropped (and would
> simplify the code). Your counterpoint is if we drop the call, then
> T-enter could do retry after retry if T-deflate is slow to get to its
> install_displaced_markword_in_object() call.
>
> If T-enter calls install_displaced_markword_in_object(), then T-enter
> will do a single retry because the object T-enter is trying to lock
> will no longer have an ObjectMonitor. Okay I finally grok it...
>
> I think we need to clarify the comment a bit:
>
> >   if (count <= 0 && _owner == DEFLATER_MARKER) {
> >     // Async deflation is in progress. Attempt to restore the
> >     // header/dmw to the object's header so that we only retry once
> >     // if the deflater thread happens to be slow.
> >     install_displaced_markword_in_object();

Done.


>
>> In my original patch no thread would ever wait for a deflating thread 
>> to finish. This property got lost in FastHashCode as that function 
>> evolved since I wrote my patch, but I think this property is worth 
>> preserving where possible. It might even be worth looking at 
>> FastHashCode to see if we can re-establish this property.
>
> Async Monitor Deflation causes races with FastHashCode() when the target
> object has an existing ObjectMonitor. Here's the base code:
>
>>   768   } else if (mark->has_monitor()) {
>>   769     monitor = mark->monitor();
>>   770     temp = monitor->header();
>> 771 assert(temp->is_neutral(), "invariant");
>>   772     hash = temp->hash();
>>   773     if (hash) {
>>   774       return hash;
>>   775     }
>>   776     // Skip to the following code to reduce code size
>
> The 'monitor' fetched on L769 is unstable due to Async Monitor
> Deflation and can cause an incorrect hash value to be returned.
> The solution is to protect the ObjectMonitor*:
>
>>   775   } else if (mark->has_monitor()) {
>> 776 ObjectMonitorHandle omh;
>> 777 if (!omh.save_om_ptr(obj, mark)) {
>> 778 // Lost a race with async deflation so try again.
>> 779 assert(AsyncDeflateIdleMonitors, "sanity check");
>> 780 goto Retry;
>> 781 }
>> 782 monitor = omh.om_ptr();
>>   783     temp = monitor->header();
>>   784     assert(temp->is_neutral(), "invariant: header=" INTPTR_FORMAT, p2i((address)temp));
>>   785     hash = temp->hash();
>>   786     if (hash != 0) {
>>   787       return hash;
>>   788     }
>>   789     // Skip to the following code to reduce code size
>
> where L776-L782 handle the protection duty and possible retry. So we
> have to protect the ObjectMonitor*, but, like enter(), we could call
> install_displaced_markword_in_object() when we retry which would limit
> T-hash to a single retry.

Done in save_om_ptr().


> ObjectSynchronizer::inflate() has a similar collision and retry issue:
>
>> 1456     // CASE: inflated
>> 1457     if (mark->has_monitor()) {
>> 1458 if (!omh_p->save_om_ptr(object, mark)) {
>> 1459 // Lost a race with async deflation so try again.
>> 1460 assert(AsyncDeflateIdleMonitors, "sanity check");
>> 1461 continue;
>> 1462 }
>
> In this situation, inflate() discovers that the object already has an
> ObjectMonitor; the object may not have had one when inflate() was
> called, but it has one now. That particular race predates this project.
>
> In any case, inflate() wants to return a stable ObjectMonitor* in the
> ObjectMonitorHandle, but if save_om_ptr() returns false, then inflate()
> has to retry. The only reason for save_om_ptr() to return false is due
> to a collision with Async Monitor Deflation. Like enter, we could call
> install_displaced_markword_in_object() when we retry which would limit
> inflate() to a single retry.

Done in save_om_ptr().


> Okay, I've evolved from thinking we could simplify the code by dropping
> install_displaced_markword_in_object() to thinking that I understand
> what install_displaced_markword_in_object() brings to the party. And now
> I'm proposing that we add 2 more install_displaced_markword_in_object()
> calls to limit retries on two more code paths.

So install_displaced_markword_in_object() is now called from:

   ObjectMonitor::enter()                         - no change
   ObjectMonitorHandle::save_om_ptr()             - on _owner == 
DEFLATER_MARKER detection
   ObjectSynchronizer::deflate_monitor_using_JT() - no change

Because of the new call in save_om_ptr(), calls to inflate() also pick up
the benefit of at most one retry. I've double checked the new loops and
the existing loops and I think all are covered.


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

Dan