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

[coleen.phillimore at oracle.com]

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, 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. But that said I would have expected the 
> monitor returned by omAlloc to be "clean" such that Recycle() should 
> not be needed either?

The whole coding style of this is not C++.  You'd expect a new operation 
like omAlloc to appear to construct a fresh object, but it looks like 
Recycle does that.   Recycle() also checks the owner is NULL with its 
assert.  It checks many of the fields it sets to NULL with the assert.

271 assert(((is_busy()|_recursions) == 0), "freeing inuse monitor");


The Recycle() code could get rid of this comment too since it's not a 
"stronger" assert than what you have.  And it sets the fields to null 
below.  Strange.

  268     // TODO: add stronger asserts ...
  269     // _cxq == 0 _succ == NULL _owner == NULL _waiters == 0
  270     // _contentions == 0 EntryList  == NULL
  271     // _recursions == 0 _WaitSet == NULL

>
> The comment is a suggestion to use assert rather than guarantee. (I 
> think the OM code has too many guarantees versus assertions.)
I agree with this!

The change looks fine to me without any further changes that I need to 
see anyway.  I made fixVersion 13 in the bug so you don't get a backport 
issue.

Coleen

>
> 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