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

[Daniel D. Daugherty]

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