Review request (m): 7160728: Introduce an extra logging level for G1 logging

[Tony Printezis]

Bengt,

Looks good, ship it.

Tony

On 04/12/2012 07:41 PM, Bengt Rutisson wrote:
>
> Hi all,
>
> Thanks Stefan, Jesper and Tony for looking at this!
>
> Here is an updated webrev that I hope include all of your comments:
> http://cr.openjdk.java.net/~brutisso/7160728/webrev.01/
>
> If I don't hear any complaints I'll go ahead and push this tomorrow.
>
> Thanks again for the reivews!
> Bengt
>
>
> On 2012-04-12 22:21, Bengt Rutisson wrote:
>>
>> Tony,
>>
>> Thanks for looking at this!
>>
>> On 2012-04-12 21:00, Tony Printezis wrote:
>>> Bengt,
>>>
>>> Thanks for doing this. It looks good. Some minor comments:
>>>
>>> g1CollectedHeap.cpp
>>>
>>> 3617     TraceTime t(verbose_str, G1Log::fine()&&  !G1Log::finer(), 
>>> true, gclog_or_tty);
>>>
>>> Is it worth introducing something like G1Log::fine_only() which 
>>> evaluates to _level == level_fine?
>>
>> I think this is the only place where it appears, so personally I 
>> don't really think it is worth it. I hope that we can clean this up a 
>> bit further in one of the other logging projects. Unless you object 
>> strongly, I'll leave it as it is.
>>
>>>
>>> g1CollectorPolicy.cpp
>>>
>>> 1029     if (G1Log::finest()) {
>>> 1030       buf.append("  %3.1lf", val);
>>> 1031     }
>>>
>>> Do you want to change %3.1lf to %.1lf to be consistent with what you 
>>> have for the avg / min / etc? (and %3.1lf should be equivalent to 
>>> %.1lf, right?)
>>
>> Good point. Fixed.
>>
>>>
>>> g1_globals.hpp
>>>
>>> 28 #include "gc_implementation/g1/g1Log.hpp" // make sure the 
>>> logging module is available to all g1 modules
>>>
>>> I would actually skip this and only include it in the appropriate 
>>> files (which will all be .cpp files) to avoid getting it compiled 
>>> for everything else.
>>
>> OK. Done.
>>
>>> universe.cpp:
>>>
>>> 896     G1Log::init();
>>>
>>> I would actually initialize this in G1CollectedHeap::initialize(). 
>>> Do we do any logging before that so that we need to initialize it 
>>> earlier?
>>
>> Yes, I was thinking about where to initialize it. I'll move it to 
>> G1CollectedHeap::initialize().
>>
>>> g1Log.hpp
>>>
>>>   31   typedef enum {
>>>   32     level_none,
>>>   33     level_fine,
>>>   34     level_finer,
>>>   35     level_finest
>>>   36   } LogLevel;
>>>
>>> I think the convention for enums is: LevelNone, LevelFine, 
>>> LevelFiner, LevelFinest?
>>
>> I don't think hotspot is really consistent about the naming for 
>> enums, but I'll change to your suggestion.
>>
>>>
>>> g1Log.cpp
>>>
>>> Is it worth turning G1LogLevel into lowercase to make the comparison 
>>> case-insensitive?
>>
>> I don't think we need to have them case insensitive. The command line 
>> option are case sensitive.
>>
>> Thanks again for looking at this. I'll send out an updated webrev 
>> shortly.
>>
>> Bengt
>>
>>>
>>> Tony
>>>
>>>
>>> On 04/11/2012 03:43 PM, Bengt Rutisson wrote:
>>>>
>>>> Hi all,
>>>>
>>>> I'd like some review for this change:
>>>> http://cr.openjdk.java.net/~brutisso/7160728/webrev.00/
>>>>
>>>> It introduces an extra logging level for G1. This is needed to do 
>>>> efficient performance analysis on G1 log files. Most of this will 
>>>> later on be replaced by a more general framework for the whole JVM.
>>>>
>>>> This is part of the work for this CR:
>>>> 7132559: G1: enhance / cleanup the PrintGCDetails output
>>>>
>>>> Thanks,
>>>> Bengt
>>
>