RFR: 8150084: Convert TraceMonitorMismatch to Unified Logging

Coleen Phillimore coleen.phillimore at oracle.com
Tue Mar 29 17:01:19 UTC 2016


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



More information about the hotspot-runtime-dev mailing list