RFR[9u-dev]: 8054326: Confusing message in "Current rem set statistics"

Fri Apr 29 13:36:03 UTC 2016

Hi,

On 2016-04-29 14:48, Cheleswer Sahu wrote:
> Hi,
>
> I made changes by replacing the "round_k()" function with byte_size_in_proper_unit() and proper_unit_for_byte_size().
> This will take care of printing the values in correct format.
>
> Please review the code changes from below link:
> Webrev: http://cr.openjdk.java.net/~csahu/8054326/webrev.01/

Looks good to me. Just one minor thing, please format this line like you 
did with the others.

  289     out->print_cr("  Total per region rem sets sizes = " 
SIZE_FORMAT "%s."
  290                   " Max = " SIZE_FORMAT "%s.",
  291                   byte_size_in_proper_unit(total_rs_mem_sz()), 
proper_unit_for_byte_size(total_rs_mem_sz()), 
byte_size_in_proper_unit(max_rs_mem_sz()),
  292                   proper_unit_for_byte_size(max_rs_mem_sz()));

cheers,
Per

>
>
> Regards,
> Cheleswer
>
>
>> -----Original Message-----
>> From: Mattis Castegren
>> Sent: Tuesday, April 19, 2016 2:45 PM
>> To: Liden; Cheleswer Sahu; Mikael Gerdin; hotspot-gc-
>> dev at openjdk.java.net
>> Cc: Mattis Castegren
>> Subject: RE: RFR[9u-dev]: 8054326: Confusing message in "Current rem set
>> statistics"
>>
>> Hi
>>
>> Sorry, maybe I misunderstood the current workings. Will the current
>> methods print bytes under 10k? I thought you suggested that we changed it
>> so that it did. I agree that is a feature, I just didn't know if it was how it
>> worked now or if it was a suggestion for further improvement.
>>
>> Kind Regards
>> /Mattis
>>
>> -----Original Message-----
>> From: Per Liden
>> Sent: den 19 april 2016 09:49
>> To: Mattis Castegren; Cheleswer Sahu; Mikael Gerdin; hotspot-gc-
>> dev at openjdk.java.net
>> Subject: Re: RFR[9u-dev]: 8054326: Confusing message in "Current rem set
>> statistics"
>>
>> Hi,
>>
>> On 2016-04-18 13:27, Mattis Castegren wrote:
>>> Ok, so the benefit of more granularity and less unclear messages for low
>> numbers stand against the ease to parse the logs.
>>>
>>> I think option 3 seems reasonable, to change both this and similar places.
>> Per, in that case, do you think we should change the
>> proper_unit_for_byte_size/byte_size_in_proper_unit functions to print
>> bytes under 10k in the same bug?
>>
>> You mean under 1K? I don't know the full history of these functions, but I
>> think people see the "print B up to 10K" as a feature and not a bug, to limit
>> the amount of rounding/loss you'll see. Is there a reason why you wouldn't
>> want that in this case too?
>>
>> cheers,
>> Per
>>
>>>
>>> How big do you all think the parsing issue will be, though? Is that so big it's a
>> stopper, or can Cheleswer go ahead and change a few of these places.
>>>
>>> Kind Regards
>>> /Mattis
>>>
>>> -----Original Message-----
>>> From: Per Liden
>>> Sent: den 13 april 2016 10:56
>>> To: Mattis Castegren; Cheleswer Sahu; Mikael Gerdin;
>>> hotspot-gc-dev at openjdk.java.net
>>> Subject: Re: RFR[9u-dev]: 8054326: Confusing message in "Current rem set
>> statistics"
>>>
>>> Hi,
>>>
>>> On 2016-04-13 09:57, Mattis Castegren wrote:
>>>> Hi
>>>>
>>>> This request came in some time ago as a low priority request from PSR
>> (the Oracle performance team):
>>>>
>>>>
>> https://bug.oraclecorp.com/pls/bug/webbug_edit.edit_info_top?rptno=19
>>>> 3
>>>> 56019
>>>> PSR:FUNC: Confusing msg in "Current rem set statistics"
>>>>
>>>> Their main complaint is that 0 usually means exactly 0. So, I think the
>> balance is between 0 being confusing when the value is non-zero vs the ease
>> of parsing the logs. I see three ways to do this:
>>>>
>>>> 1) Close the bug as Won't Fix
>>>> 2) Change this specific issue, print bytes if the value is lower than
>>>> 1k
>>>> 3) Change this and a few other places.
>>>>
>>>> We in Sustaining don't have too strong of an opinion here. Stanley Guan
>> from PSR recently commented the BugDB bug making the case for changing
>> the output, but if the GC team is of the opinion that we should close this as
>> Won't Fix with the motivation that parsing becomes harder with this change,
>> then we can close the bug.
>>>>
>>>> Anyone else who has an opinion or a motivation for any of the options
>> above?
>>>
>>> I would prefer 1 or 3, where 3 is be using the existing
>> proper_unit_for_byte_size/byte_size_in_proper_unit functions, unless
>> there's a good reason not to. I can't see any problems with printing B for sizes
>> under 10K rather than 1K, it's actually a feature.
>>>
>>> cheers,
>>> Per
>>>
>>>>
>>>> Kind Regards
>>>> /Mattis
>>>>
>>>> -----Original Message-----
>>>> From: Cheleswer Sahu
>>>> Sent: den 11 april 2016 15:33
>>>> To: Liden; Mikael Gerdin; hotspot-gc-dev at openjdk.java.net
>>>> Cc: Mattis Castegren
>>>> Subject: RE: RFR[9u-dev]: 8054326: Confusing message in "Current rem
>> set statistics"
>>>>
>>>> Hi,
>>>>
>>>>
>>>> -----Original Message-----
>>>> From: Per Liden
>>>> Sent: Friday, April 08, 2016 1:57 PM
>>>> To: Mikael Gerdin; Cheleswer Sahu; hotspot-gc-dev at openjdk.java.net
>>>> Subject: Re: RFR[9u-dev]: 8054326: Confusing message in "Current rem
>> set statistics"
>>>>
>>>> Hi,
>>>>
>>>> On 2016-04-06 13:09, Mikael Gerdin wrote:
>>>> [...]
>>>>>> Hi,
>>>>>>
>>>>>>
>>>>>> Please review the code changes for
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8054326.
>>>>>>
>>>>>>
>>>>>> Webrev Link: http://cr.openjdk.java.net/~csahu/8054326/
>>>>>>
>>>>
>>>> We try to avoid printing sizes with dynamically selected binary
>>>> prefixes
>>>> (K/M/G) for the simple reason that it makes it harder to grep through and
>> compare GC logs. If that "rule" applies here can be discussed.
>>>>
>>>> We already have the functions
>>>> proper_unit_for_byte_size/byte_size_in_proper_unit to do roughly what
>> you've done, but they are very rarely used for the reason stated above.
>>>>
>>>> Mikael had directed me towards this function when we had offline chat
>> regarding this, but this function prints in KB only if size is  greater than 10k.
>> Therefore I had written my own code to print this.
>>>>
>>>> There are a other places in this code/function where we're also printing
>> potentially small values as K, which you haven't addressed here. Just
>> addressing one of the places seems like an incomplete fix.
>>>>
>>>> Yes, I agree there are other places too which can be considered. I just
>> took the "Max" part because I was not sure about others.
>>>>
>>>> I'm personally not sure if this is a problem big enough to worth addressing
>> at all. Maybe others have a different opinion, Thomas/Bengt/Mikeal/Stefan?
>>>>
>>>> Does any have any other opinion regarding this ? We in sustaining team
>> although feel that showing "Max" as 0k is ambiguous.
>>>>
>>>>
>>>> Regards,
>>>> Cheleswer
>>>> cheers,
>>>> Per
>>>>
>>>>>>
>>>>>> Bug Brief: In output of remset statistics "Max"  is printing as 0k,
>>>>>> which is confusing for user.
>>>>>>
>>>>>>
>>>>>>
>>>>>> Problem Identified : "Max" value is rounded to KB, which result  in
>>>>>> 0k, if the value is less than 1024B.
>>>>>>
>>>>>>
>>>>>>
>>>>>> Solution Proposed:  If the value for "Max" is less than 1 KB (1024
>>>>>> B),  print the value in bytes (i.e. 1023B.) else
>>>>>>
>>>>>> print the value in KB.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> Regards,
>>>>>>
>>>>>> Cheleswer
>>>>>>
>>>>>>
>>>>>>