RFR(S/M): 8217659 monitor_logging updates from Async Monitor Deflation project

[Daniel D. Daugherty]

On 1/28/19 5:15 PM, coleen.phillimore at oracle.com wrote:
> On 1/28/19 5:07 PM, Daniel D. Daugherty wrote:
>> On 1/28/19 4:32 PM, coleen.phillimore at oracle.com wrote:
>>>
>>> Hi Dan, This looks better, especially audit_and_print_stats.
>>
>> Thanks! And thanks for the re-review!!
>>
>>
>>> My eyes are terrible, but it looks like this is 
>>> ObjectMonitor::verify_free()
>>
>> This comment really confused me. I didn't remember an
>> ObjectMonitor::verify_free() so I was grep'ing all over the
>> place trying to find it.
>>
>> After reading to the bottom of this email, I see that you
>> think this could be refactored into a new function called
>> ObjectMonitor::verify_free(). Now I feel less confused... :-)
>>
>
> Sorry about that.  It was my second (better, if you can believe it) 
> attempt at suggesting this.

It was a better attempt.


>>
>>> + if (n->is_busy()) {
>>> + out->print_cr("ERROR: monitor=" INTPTR_FORMAT ": free global 
>>> monitor "
>>> + "must not be busy.", p2i(n));
>>> + *error_cnt_p = *error_cnt_p + 1;
>>> + }
>>> + if (n->header() != NULL) {
>>> + out->print_cr("ERROR: monitor=" INTPTR_FORMAT ": free global 
>>> monitor "
>>> + "must have NULL _header field: _header=" INTPTR_FORMAT,
>>> + p2i(n), p2i(n->header()));
>>> + *error_cnt_p = *error_cnt_p + 1;
>>> + }
>>> + if (n->object() != NULL) {
>>> + out->print_cr("ERROR: monitor=" INTPTR_FORMAT ": free global 
>>> monitor "
>>> + "must have NULL _object field: _object=" INTPTR_FORMAT,
>>> + p2i(n), p2i(n->object()));
>>> + *error_cnt_p = *error_cnt_p + 1;
>>> + } ...
>>>
>>> And this is ObjectMonitor::verify_in_use()
>>>
>>> + if (n->header() == NULL) {
>>> + out->print_cr("ERROR: monitor=" INTPTR_FORMAT ": in-use global 
>>> monitor "
>>> + "must have non-NULL _header field.", p2i(n));
>>> + *error_cnt_p = *error_cnt_p + 1;
>>> + }
>>> + if (n->object() == NULL) {
>>> + out->print_cr("ERROR: monitor=" INTPTR_FORMAT ": in-use global 
>>> monitor "
>>> + "must have non-NULL _object field.", p2i(n));
>>> + *error_cnt_p = *error_cnt_p + 1;
>>> + } ...
>>
>> And another refactor recommendation... okay.
>>
>>
>>> And you do the same checks for the per-thread versions.  That's what 
>>> I noticed before.  I like the refactoring that you did but if you 
>>> want to do more, this is what I'd suggest.
>>
>> In my first reply to David on this review thread, I wrote:
>>
>>> Maybe after the above simplifications, other ways to make this
>>> function more digestible will become apparent... 
>>
>> What you're suggesting fits right in with more simplifications
>> becoming apparent.
>
> Yes, the power of refactoring ...

Fits right in there with the power of code deletion.


>
>>> ObjectMonitor doesn't have any verify functions and it seems like 
>>> the knowledge of these fields really belongs there.  Anyways, these 
>>> files are tightly coupled, so it's ok the way it is.  Maybe better 
>>> not to pull that thread right now.
>>
>> It's kind of a toss up between ObjectMonitor.* and synchronizer.*.
>> Yes, ObjectMonitor.* defines all the fields and _some_ of the ops,
>> but there is a lot of logic in synchronizer.* that determines how
>> the fields are used.
>
> That's sort of broken encapsulation imho, which makes this tricky code 
> more difficult to understand.  We can clean up as we go though.  
> There's plenty to do in that regard.

Preaching to the choir... The purpose of this bug fix
is to make the code easier to diagnose since we are
considering making changes to it. We will definitely
clean up as we go.


>>
>> I'll take a look at the possibility of more refactoring, but it
>> may not happen with this bug (8217659).
>>
>
> Yes, if you want to do this separately and have captured enough of 
> this to do so, a separate RFE is fine.  I'll add to it if you need me to.

I think I could file a nice RFE to cover this idea if I don't
take care of it now.

Dan


>
> Thanks,
> Coleen
>> Dan
>>
>>
>>>
>>> thanks,
>>> Coleen
>>>
>>>
>>>
>>> On 1/26/19 10:10 PM, Daniel D. Daugherty wrote:
>>>> Greetings,
>>>>
>>>> I've updated the patch to resolve code review comments from David H.
>>>> and Coleen. I've also rebased the patch to the jdk/jdk repo at the
>>>> 'jdk-13+5' tag (plus the 8217658 patch); there were no merge 
>>>> surprises.
>>>>
>>>> Here's an incremental webrev:
>>>>
>>>> http://cr.openjdk.java.net/~dcubed/8217659-webrev/1-for-jdk13.inc/
>>>>
>>>> I've also generated a full webrev since the changes to 
>>>> synchronizer.cpp
>>>> are extensive and that's the easier way to review it:
>>>>
>>>> http://cr.openjdk.java.net/~dcubed/8217659-webrev/1-for-jdk13.full/
>>>>
>>>> This patch along with 8217658 have been through Mach5
>>>> builds-tier1,hs-tier1,jdk-tier1,hs-tier2,hs-tier3 testing with
>>>> no failures.
>>>>
>>>> Thanks, in advance, for any questions, comments or suggestions.
>>>>
>>>> Dan
>>>>
>>>> P.S.
>>>> I've attached sample output for running Hello.java with the
>>>> monitorinflation logging enabled at Info, Debug and Trace.
>>>>
>>>>
>>>> On 1/23/19 3:16 PM, Daniel D. Daugherty wrote:
>>>>> Greetings,
>>>>>
>>>>> I have a (S)mall/(M)edium patch extracted from the Async Monitor 
>>>>> Deflation
>>>>> project that is ready for code review. I'm calling this a 
>>>>> (S)mall/(M)edium
>>>>> because the logic changes are (S)mall, but the logging code is 
>>>>> tedious and
>>>>> there's a bunch of it in audit_and_print_stats() so (M)edium.
>>>>>
>>>>> The short version of what this patch is about:
>>>>>
>>>>>     This sub-task captures updates and additions to the
>>>>>     baseline monitor logging code.
>>>>>
>>>>> The details are in the bug report:
>>>>>
>>>>>     JDK-8217659 monitor_logging updates from Async Monitor 
>>>>> Deflation project
>>>>>     https://bugs.openjdk.java.net/browse/JDK-8217659
>>>>>
>>>>> Here's the webrev:
>>>>>
>>>>> http://cr.openjdk.java.net/~dcubed/8217659-webrev/0-for-jdk13/
>>>>>
>>>>> This patch along with the other two patches for Async Monitor 
>>>>> Deflation
>>>>> project have been through Mach5 
>>>>> builds-tier1,hs-tier1,jdk-tier1,hs-tier2,hs-tier3
>>>>> testing and I'm currently running my stress testing kits on my 
>>>>> Linux-X64
>>>>> and Solaris-X64 servers.
>>>>>
>>>>> I have been actively using this new logging code while debugging my
>>>>> port of the Async Monitor Deflation project code.
>>>>>
>>>>> Thanks, in advance, for any questions, comments or suggestions.
>>>>>
>>>>> Dan
>>>>>
>>>>>
>>>>
>>>
>>
>