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

[Bengt Rutisson]

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
>