Code review request: 6855834: G1: minimize the output when -XX:+PrintHeapAtGC is set (S)

[Tony Printezis]

John,

Thanks, see inline.

John Coomes wrote:
> Tony Printezis (Antonios.Printezis at Sun.COM) wrote:
>   
>> One more spin:
>>
>> http://cr.openjdk.java.net/~tonyp/6855834/webrev.2/
>>
>> All the recent changes are in g1CollectedHeap.cpp, in the print_on() 
>> method (which reflect the updated output that John Coomes suggested). 
>> One more review and I'll push it...
>>     
>
> Looks good ... except for some whitespace nits in g1CollectedHeap.cpp
> (sorry):
>   
No need to apologize! :-)
> 1.  if (PrintHeapAtGC){    <---- no space before the '{' 
>
> 	This occurs 4 times (I think).
>   
Yep, again bad cut-and-paste.
> 2.  No blank line between methods print(), print_on(outputStream*) and
> print_on(outputStream*, bool) (lines 2336, 2339).
>   
I'll add them, but it's the same in genCollectedHeap.cpp:

void GenCollectedHeap::print() const { print_on(tty); }
void GenCollectedHeap::print_on(outputStream* st) const {
  for (int i = 0; i < _n_gens; i++) {
    _gens[i]->print_on(st);
  }
  perm_gen()->print_on(st);
}
> 3.  No space between the double quote and INTPTR_FORMAT or
> SIZE_FORMAT in the new print_on() code.  AFAICT, most of hotspot uses
> this:
>
> 	print("count = " SIZE_FORMAT "K", count);
>
> instead of
>
> 	print("count = "SIZE_FORMAT"K", count);
>
> I find the latter harder to parse.  I see some existing examples of
> the latter in the same file, but haven't seen it outside of G1.
>   
I personally find the second version easier to parse, given that it's a 
bit clearer where there's white space and where there's not. I'll change 
it in an attempt to be consistent though.

Tony
>> Tony Printezis wrote:
>>     
>>> Thanks to Ramki for the speedy code review. I applied a couple of 
>>> minor changes, here's the second attempt:
>>>
>>> http://cr.openjdk.java.net/~tonyp/6855834/webrev.1/
>>>
>>> Tony
>>>
>>> Tony Printezis wrote:
>>>       
>>>> http://cr.openjdk.java.net/~tonyp/6855834/webrev.0/
>>>>
>>>> The CR has more information on the new format:
>>>>
>>>> Tony
>>>>
>>>>         
>> -- 
>> ---------------------------------------------------------------------
>> | Tony Printezis, Staff Engineer   | Sun Microsystems Inc.          |
>> |                                  | MS UBUR02-311                  |
>> | e-mail: tony.printezis at sun.com   | 35 Network Drive               |
>> | office: +1 781 442 0998 (x20998) | Burlington, MA 01803-2756, USA |
>> ---------------------------------------------------------------------
>> e-mail client: Thunderbird (Linux)
>>
>>
>>     
>
>   

-- 
----------------------------------------------------------------------
| Tony Printezis, Staff Engineer    | Sun Microsystems Inc.          |
|                                   | MS BUR02-311                   |
| e-mail: tony.printezis at sun.com    | 35 Network Drive               |
| office: +1 781 442 0998 (x20998)  | Burlington, MA01803-0902, USA  |
----------------------------------------------------------------------
e-mail client: Thunderbird (Solaris)