RFR: 8139564: Convert TraceDefaultMethods to Unified Logging

[harold seigel]

Hi Rachel,

I agree that the ResourceMark at line 940 should be left in.

Thanks, Harold

On 10/16/2015 5:32 PM, Rachel Protacio wrote:
> Thanks for the review.
>
> On 10/16/2015 12:13 PM, harold seigel wrote:
>> Hi Rachael,
>>
>> The code looks good.
>>
>> One comment about this FOR loop in defaultMethods.cpp:
>>
>>      686     for (int i = 0; i < slots->length(); ++i) {
>>    687 ResourceMark rm;
>>    688 outputStream* logstream = 
>> LogHandle(defaultmethods)::debug_stream();
>>    689 streamIndentor si(logstream);
>>    690 logstream->indent();
>>    691 slots->at(i)->print_on(logstream);
>>    692 logstream->cr();
>>      693     }
>>
>> Can you move lines 687-689 above the loop so that they only get 
>> executed once per FOR loop, instead of every iteration?
>>
> Good idea.
>>
>> Also, you may not need the ResourceMark at line 831 because there is 
>> one at line 809.
>>
>>    830 if (log_is_enabled(Debug, defaultmethods)) {
>>    831 ResourceMark rm;
>>    832 outputStream* logstream = 
>> LogHandle(defaultmethods)::debug_stream();
>>    833 streamIndentor si(logstream, 2);
>>    834 logstream->indent().print("Looking for default methods for 
>> slot ");
>>    835 slot->print_on(logstream);
>>    836 logstream->cr();
>>
> Also done.
>>
>> Finally, is the ResourceMark needed at line 940?
>>
> Well, every LogHandle stream needs a ResourceMark, which is why I 
> included it there and in line 831. As you pointed out, the latter one 
> has a ResourceMark in the code above it. Although technically the one 
> at 940 is currently only called within the scope of the 809 
> ResourceMark, since it is in a separate function, it's probably safer 
> to leave it. What do you think?
>
> Thanks,
> Rachel
>> Thanks, Harold
>>
>> On 10/15/2015 6:33 PM, Ioi Lam wrote:
>>>
>>>
>>> On 10/15/15 10:51 AM, Rachel Protacio wrote:
>>>> Hi, Ioi,
>>>>
>>>> Thanks for the comments. While all valid points, the decision by 
>>>> the serviceability team with regards to the logging framework as a 
>>>> whole is to move all the output to product mode. Because of this, I 
>>>> ran performance tests to make sure that the newly-introduced 
>>>> product code will not slow it down. So yes, all the "#ifndef 
>>>> PRODUCT" sections that are necessary for this logging have been 
>>>> liberated to product mode.
>>>>
>>> Thanks Rachel. This makes sense.
>>>
>>> - Ioi
>>>
>>>> Also, I realized I did not remove the TraceDefaultMethods flag from 
>>>> globals.hpp, so here is the link to the updated webrev: 
>>>> http://cr.openjdk.java.net/~rprotacio/8139564.01/
>>>> Which builds appropriately. The change now encompasses all the 
>>>> references to TraceDefaultMethods. A compatibility request has been 
>>>> accepted with regards to this change.
>>>>
>>>> Thanks,
>>>> Rachel
>>>>
>>>> On 10/14/2015 11:58 PM, Ioi Lam wrote:
>>>>> Hi Rachel,
>>>>>
>>>>> Before your changes, this block of code  would be excluded from 
>>>>> product builds:
>>>>>
>>>>>  684 #ifndef PRODUCT
>>>>>  685   if (TraceDefaultMethods) {
>>>>>  686     tty->print_cr("Slots that need filling:");
>>>>>  687     streamIndentor si(tty);
>>>>>  688     for (int i = 0; i < slots->length(); ++i) {
>>>>>  689       tty->indent();
>>>>>  690       slots->at(i)->print_on(tty);
>>>>>  691       tty->cr();
>>>>>  692     }
>>>>>  693   }
>>>>>  694 #endif // ndef PRODUCT
>>>>>
>>>>> but after your change, it will be included in product builds. This 
>>>>> means product builds will have more verbose output than before. It 
>>>>> also means that the product builds will get bigger (because some 
>>>>> printing code, such as EmptyVtableSlot::print_on(), would need to 
>>>>> be enabled for product builds as well).
>>>>>
>>>>> I am not very familiar with UL so maybe this is an FAQ ... while 
>>>>> doing the UL conversion, should we add all the old "ifndef 
>>>>> PRODUCT" logs into the product build?
>>>>>
>>>>> Thanks
>>>>> - Ioi
>>>>>
>>>>> On 10/14/15 7:10 PM, Rachel Protacio wrote:
>>>>>> Hello! Please take a look at my enhancement, the first of the 
>>>>>> runtime logging flags to be converted.
>>>>>>
>>>>>> Summary: The former -XX:+TraceDefaultMethods flag is updated to 
>>>>>> the unified logging framework and is now replaced with 
>>>>>> -Xlog:defaultmethods.
>>>>>>
>>>>>> open webrev: http://cr.openjdk.java.net/~rprotacio/8139564/
>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8139564
>>>>>> testing: Passes JPRT, RBT, and RefWorkload performance testing.
>>>>>>
>>>>>> Thank you,
>>>>>> Rachel
>>>>>
>>>>
>>>
>>
>