RFR: 8150084: Convert TraceMonitorMismatch to Unified Logging

[Coleen Phillimore]

Thumbs up!
Thanks for checking
Coleen

On 3/29/16 2:13 PM, Max Ockner wrote:
> This does not need @modules. According to Harold, if the test passes 
> without @modules, then it doesn't need @modules.
> Max
>
> On 3/29/2016 1:01 PM, Coleen Phillimore wrote:
>>
>> This looks good - but one last question (sorry for incremental 
>> comments).
>>
>> http://cr.openjdk.java.net/~mockner/8150084.04/test/runtime/logging/MonitorMismatchTest.java.html 
>>
>>
>> Does this need any @modules things?
>>
>> Thanks,
>> Coleen
>>
>> On 3/29/16 12:56 PM, Max Ockner wrote:
>>> Fixed: http://cr.openjdk.java.net/~mockner/8150084.04/
>>>
>>> Thanks,
>>> Max
>>> On 3/28/2016 5:47 PM, Coleen Phillimore wrote:
>>>>
>>>> MonitorMismatchHelper.java needs a copyright notice.
>>>> Coleen
>>>>
>>>> On 3/28/16 4:15 PM, Max Ockner wrote:
>>>>> Thanks for round 1 of reviews.
>>>>>
>>>>> webrev: http://cr.openjdk.java.net/~mockner/8150084.03/
>>>>> - Renamed A.jasm to MonitorMismatchHelper.jasm and included it in 
>>>>> webrev.
>>>>> - Fixed improper spacing in test.
>>>>>
>>>>> Thanks,
>>>>> Max
>>>>> On 3/28/2016 3:52 PM, Coleen Phillimore wrote:
>>>>>>
>>>>>> It looks like A.jasm is missing from your webrev and can you name 
>>>>>> it something more useful like MonitorMismatchHelper.java ?
>>>>>> Otherwise, code looks good.
>>>>>> thanks,
>>>>>> Coleen
>>>>>>
>>>>>> On 3/28/16 3:10 PM, Max Ockner wrote:
>>>>>>> Good catch. We don't intend for this to be DEBUG_ONLY. The 
>>>>>>> "#ifndef PRODUCT" has been removed.
>>>>>>> Webrev: http://cr.openjdk.java.net/~mockner/8150084.02/
>>>>>>>
>>>>>>> If we put the log_is_enabled conditional inside the function 
>>>>>>> then we will always branch to the function. The conversion of 
>>>>>>> TraceClassResolution required a similar decision, and we chose 
>>>>>>> to leave the log_is_enabled condition outside of the 
>>>>>>> trace_class_resolution() function.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Max
>>>>>>>
>>>>>>> On 3/25/2016 2:18 PM, Rachel Protacio wrote:
>>>>>>>> Hey, Max,
>>>>>>>>
>>>>>>>> The code looks fine as-is, but perhaps could be optimized? 
>>>>>>>> Since all the uses of monitormismatch call 
>>>>>>>> report_monitor_mismatch(), which has its code in non-product 
>>>>>>>> only, you could remove the conditionals around it and have the 
>>>>>>>> function declared with "PRODUCT_RETURN." Then, inside the 
>>>>>>>> function you could put the "if log_is_enabled". So there will 
>>>>>>>> be less compiled code in product mode. (This might also mean 
>>>>>>>> you could make the monitormismatch tag "DEBUG_ONLY", but I'm 
>>>>>>>> not 100% sure about how that works.)
>>>>>>>>
>>>>>>>> Rachel
>>>>>>>>
>>>>>>>> On 3/25/2016 1:28 PM, Max Ockner wrote:
>>>>>>>>> Hello,
>>>>>>>>> Please review another Unified Logging conversion.
>>>>>>>>>
>>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8150084
>>>>>>>>> Webrev: http://cr.openjdk.java.net/~mockner/8150084.01/
>>>>>>>>>
>>>>>>>>> TraceMonitorMismatch has been converted to unified logging 
>>>>>>>>> with monitormismatch tag and info level. There is very little 
>>>>>>>>> output. To trigger the output, I ran a program which does a 
>>>>>>>>> monitorenter without a monitorexit.
>>>>>>>>>
>>>>>>>>> 'java -XX:+TieredCompilation -Xcomp -Xlog:monitormismatch 
>>>>>>>>> <program>'
>>>>>>>>>
>>>>>>>>> Tested with jtreg runtime tests and added 
>>>>>>>>> MonitorMismatchTest.java to test for the output. The test body 
>>>>>>>>> doesn't run on embedded since it uses TieredCompilation.
>>>>>>>>>
>>>>>>>>> If you search for TraceMonitorMismatch in the new source, you 
>>>>>>>>> will find a bunch of instances in 
>>>>>>>>> jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/GenerateOopMap.java. 
>>>>>>>>> This file is a java implementation of GenerateOopMap.cpp and 
>>>>>>>>> the two files aren't tied together in any way.  This java 
>>>>>>>>> level TraceMonitorMismatch flag is set to true in the code and 
>>>>>>>>> doesn't get its value from the jvm level TraceMonitorMismatch 
>>>>>>>>> flag. I have left these alone as another unfortunate naming 
>>>>>>>>> coincidence.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Max
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>