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

[coleen.phillimore at oracle.com]

On 6/12/19 10:32 AM, Daniel D. Daugherty wrote:
> 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.

Thumbs up.  I'm glad you're doing this!
thanks,
Coleen

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