RFR(XXXS): 7092245: G1: Wrong format specifier in G1PrintRegionLivenessInfo header output

[Ramki Ramakrishna]

Hi Tony --

On 9/21/2011 9:51 AM, Tony Printezis wrote:
> John,
>
> Looks good.
>
> Ramki and Bengt,
>
> The format of this output looks a bit like this:
>
> ### PHASE Post-Sorting @ 5.303
> ### HEAP  committed: 0xf2400000-0xf6400000  reserved: 
> 0xf2400000-0xf6400000  region-size: 1048576
> ###
> ###   type         address-range       used  prev-live  
> next-live          gc-eff
> ###   OLD  0xf4d00000-0xf4e00000    1048576        680          
> 0       1036338.1
> ###   OLD  0xf4800000-0xf4900000    1048576        824          
> 0       1033205.5
> ...
> ###   OLD  0xf5e00000-0xf5f00000    1048576    1043552          
> 0           747.1
> ###
> ### SUMMARY  capacity: 35.00 MB  used: 34.51 MB / 98.60 %  prev-live: 
> 12.81 MB / 36.59 %  next-live: 0.00 MB / 0.00 %
>
> So, the per-region information is in bytes, the summary is in MBs. 
> It's nice to have the accuracy in the per-region info (e.g., in the 
> above example, 680 bytes will probably be lost in the rounding). I 
> vote to leave it as is. Also, note that region size is bounded (to 
> 16MB), so the byte sizes are also bounded.

OK, in context, this seems fine. Below, a related comment ...

As regards, the sizes in bytes, and the efficiency, would it be a good 
idea to provide the units?
(Is efficiency in bytes/ms?). Perhaps they are for internal consumption 
and left without units
explicitly stated (with the understanding that if you don't know the 
units you should not be
reading this log), but if this is for general user consumption, a header 
line somewhere with
the unit would not hurt :-)

But this of course does not have to be done now.

-- ramki

>
> Tony
>
> Bengt Rutisson wrote:
>>
>> John,
>>
>> Looks good.
>>
>> I agree with Ramki that it would have been nice to have the size 
>> value in an appropriate unit (especially considering that the minimum 
>> size i 1 MB and the size will always be a multiple of 1 MB). If you 
>> feel that such a change is outside the scope of this CR I am fine 
>> with your change as it is.
>>
>> Bengt
>>
>> On 2011-09-21 00:31, Y. S. Ramakrishna wrote:
>>> Would it be a good idea to provide size in KB or MB, to make
>>> the output more concise and easier for human consumption?
>>>
>>> Your change looks fine though.
>>> -- ramki
>>>
>>> On 09/20/11 15:22, John Cuthbertson wrote:
>>>> Hi Everyone,
>>>>
>>>> Can I have a review of the tiny fix for this CR? The webrev can be 
>>>> found at: http://cr.openjdk.java.net/~johnc/7092245/webrev.0/
>>>>
>>>> Summary: In the G1PrintRegionLivenessInfo output, the region size 
>>>> in the header is printed using a SIZE_FORMAT but is typed as an 
>>>> int. That can cause some incorrect output by the 64-bit VM on some 
>>>> systems. The solution is to cast the value of 
>>>> HeapRegion::GrainBytes to a size_t in the print statement.
>>>>
>>>> Tested using gcbasher:
>>>>
>>>> Old output:
>>>> ### PHASE Post-Marking @ 1.481
>>>> ### HEAP  committed: 0x00000000eae00000-0x00000000f4d00000  
>>>> reserved: 0x00000000eae00000-0x00000000fae00000  region-size: 
>>>> 139672337514496
>>>> ###
>>>>
>>>> New output:
>>>> ### PHASE Post-Marking @ 1.480
>>>> ### HEAP  committed: 0x00000000eae00000-0x00000000f4d00000  
>>>> reserved: 0x00000000eae00000-0x00000000fae00000  region-size: 1048576
>>>> ###
>>>>
>>>> Thanks,
>>>>
>>>> JohnC
>>