RFR(XS): 8225453: is_busy diagnostics and other baseline cleanups from Async Monitor Deflation project

[Daniel D. Daugherty]

Greetings,

Just to close the loop on this...

Due to this assert() in Recycle():

     assert((is_busy() | _recursions) == 0, "freeing in-use monitor: %s, "
            "recursions=" INTPTR_FORMAT, is_busy_to_string(&ss), 
_recursions);

I'm able to delete two different guarantee() calls:

$ hg diff
diff -r 7661fb3c877d src/hotspot/share/runtime/synchronizer.cpp
--- a/src/hotspot/share/runtime/synchronizer.cpp    Tue Jun 11 20:32:31 
2019 -0400
+++ b/src/hotspot/share/runtime/synchronizer.cpp    Wed Jun 12 10:42:27 
2019 -0400
@@ -1065,7 +1065,6 @@
          ObjectMonitor * take = gFreeList;
          gFreeList = take->FreeNext;
          guarantee(take->object() == NULL, "invariant");
-        guarantee(!take->is_busy(), "must be !is_busy: %s", 
take->is_busy_to_string(&ss));
          take->Recycle();
          omRelease(Self, take, false);
        }
@@ -1472,8 +1471,6 @@
      // prepare m for installation - set monitor to initial state
      m->Recycle();
      m->set_header(mark);
-    guarantee(m->_owner == NULL,
-              "unexpected owner value: " INTPTR_FORMAT, p2i(m->_owner));
      m->set_object(object);
      m->_Responsible  = NULL;
      m->_SpinDuration = ObjectMonitor::Knob_SpinLimit;       // 
consider: keep metastats by type/class

And the above diff just happens to show the nearby Recycle() calls.

Thanks again for the reviews!

Dan


On 6/12/19 10:32 AM, Daniel D. Daugherty wrote:
> Hi David,
>
> Thanks for the quick review. More below...
>
>
> On 6/11/19 11:08 PM, David Holmes wrote:
>> Hi Dan,
>>
>> On 12/06/2019 9:28 am, Daniel D. Daugherty wrote:
>>> Greetings,
>>>
>>> I have one last set of baseline cleanup changes extracted from the
>>> Async Monitor Deflation project (JDK-8153224):
>>>
>>>      JDK-8225453 is_busy diagnostics and other baseline cleanups 
>>> from Async
>>>                  Monitor Deflation project
>>>      https://bugs.openjdk.java.net/browse/JDK-8225453
>>>
>>> Please see the bug report for a detailed description of the changes.
>>
>> Thanks for all the details! Made it much easier to evaluate.
>>
>>> Here's the webrev URL:
>>>
>>> http://cr.openjdk.java.net/~dcubed/8225453-webrev/0-for-jdk-jdk13/
>>
>> This is fine to push as-is,
>
> Thanks!
>
>
>> but I have one query and one comment:
>>
>> src/hotspot/share/runtime/synchronizer.cpp
>>
>>       ObjectMonitor * m = omAlloc(Self);
>>       // prepare m for installation - set monitor to initial state
>>       m->Recycle();
>>       m->set_header(mark);
>> -     m->set_owner(NULL);
>> +     guarantee(m->_owner == NULL,
>> +               "unexpected owner value: " INTPTR_FORMAT, 
>> p2i(m->_owner));
>>
>> Query: I can't quite determine where we ensure that m->_owner will be 
>> NULL. I would presume that comes from omAlloc either returning a 
>> fresh OM or else a recycled one - for which _owner must be NULL else 
>> it couldn't have been recycled.
>
> First the context: We are in ObjectSynchronizer::inflate() in
> the "CASE: neutral" part of the code. We have called omAlloc()
> and m->Recycle().
>
> Simplified view of from where omAlloc() gets an ObjectMonitor:
>   - allocate from per-thread free list if possible
>   - allocate from global free list if possible
>   - allocate more ObjectMonitors
>
> When allocating fresh ObjectMonitors, we zero them:
>
> src/hotspot/share/runtime/synchronizer.cpp
>
>     void* real_malloc_addr = (void *)NEW_C_HEAP_ARRAY(char, aligned_size,
>                                                       mtInternal);
>     temp = (PaddedEnd<ObjectMonitor> *)
>              align_up(real_malloc_addr, DEFAULT_CACHE_LINE_SIZE);
> <snip>
>     if (temp == NULL) {
>       vm_exit_out_of_memory(neededsize, OOM_MALLOC_ERROR,
>                             "Allocate ObjectMonitors");
>     }
>     (void)memset((void *) temp, 0, neededsize);
>
> so making a claim about the owner field being NULL is easy.
>
> For the two free lists, an ObjectMonitor only gets on a free list
> when it becomes !is_busy() (which includes checking the owner field)
> and is deflated at a safepoint. So in the safepoint based deflation
> world, we can safely make the claim that an ObjectMonitor that we
> get from a free list also has a NULL owner field.
>
>
> Now for the kicker. This assert is in Recycle():
>
>     assert((is_busy() | _recursions) == 0, "freeing in-use monitor: %s, "
>            "recursions=" INTPTR_FORMAT, is_busy_to_string(&ss), 
> _recursions);
>
> is_busy() includes checking the owner field so we don't need to
> guarantee() or assert() the owner field. I'll remove the guarantee().
>
>
>> But that said I would have expected the monitor returned by omAlloc 
>> to be "clean" such that Recycle() should not be needed either?
>
> The role of Recycle() (or the role of clear()) is not clear (no pun
> intended and IMHO). I'm trying to clean all this up and this set of
> changes in an attempt the detangle the mess in a bite size chunk.
> There will be more cleanups (but not for JDK13).
>
>
>> The comment is a suggestion to use assert rather than guarantee.
>
> Because of the assert() in Recycle(), that guarantee() can go away.
>
>
>> (I think the OM code has too many guarantees versus assertions.)
>
> Yes it does. That's another thing on my cleanup list. Robbin has also
> requested work in this area.
>
> Thanks again for the quick review.
>
> Dan
>
>
>>
>> Thanks,
>> David
>>
>>> The webrev is relative to jdk-13+24 and I'm hoping to get this fix
>>> into jdk/jdk this week for the last snapshot before the JDK13 fork.
>>>
>>> This fix was included in my latest round of JDK-8153324 testing which
>>> included Mach5 Tier[1-8], my stress kit on Linux-X64 and Solaris-X64,
>>> my Inflate2 test running in parallel with Kitchensink8H on macOSX,
>>> Linux-X64 and Solaris-X64. I also ran the Inflate2 and Kitchensink8H
>>> combo in parallel on the jdk-13+24 baseline + this fix on macOSX,
>>> Linux-X64 and Solaris-X64. No regressions were observed in any of
>>> the testing.
>>>
>>> Thanks, in advance, for any comments, suggestions or questions.
>>>
>>> Dan
>
>