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

[Daniel D. Daugherty]

Coleen,

Thanks for the quick review. More below...


On 6/12/19 8:56 AM, coleen.phillimore at oracle.com wrote:
> 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++. 

Yes that observation has been made before.


> You'd expect a new operation like omAlloc to appear to construct a 
> fresh object,
> but it looks like Recycle does that.

As I wrote to David, the role of Recycle() (and clear()) is not clear.
I'm cleaning that up, one bite sized chunk at a time.


> 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");

Since Recycle() assert()'s that the owner field is NULL, I'm going to
get rid of the guarantee() about owner in inflate().

The how, the where, and the why of setting various fields in the
ObjectMonitor and assert()'ing specific values needs to be cleaned
up. I'm working on it.


> 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

I have been deleting stale comments after appropriate research and
checking. This one is on my list to visit when I get to the task
of "normalizing" Recycle() and clear() and how they relate to
omAlloc().


>
>>
>> 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 guarantee() is being deleted due to the assert() in Recyle().


> The change looks fine to me without any further changes that I need to 
> see anyway.

Thanks.


> I made fixVersion 13 in the bug so you don't get a backport issue.

Thanks for taking care of that. I would have probably missed updating
the triage label.

Dan


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