[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Re: RFR: 8150084: Convert TraceMonitorMismatch to Unified Logging
From:       Max Ockner <max.ockner () oracle ! com>
Date:       2016-03-29 18:29:07
Message-ID: 56FAC973.7040505 () oracle ! com
[Download RAW message or body]

OK I will submit this now. Thanks!
On 3/29/2016 2:25 PM, Coleen Phillimore wrote:
> 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
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

[prev in list] [next in list] [prev in thread] [next in thread]