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

[Bengt Rutisson]

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