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

Wed May 4 09:38:41 UTC 2016

Hi Cheleswer, yes I think that looks good.  Will help with push, as 
discussed you've changed that long line/formatting as suggested.

Thanks
Kevin

On 02/05/2016 05:41, Cheleswer Sahu wrote:
> Hi,
>
> Thanks Per for review. I will correct the formatting  before pushing the changes.
>
>
> Regards,
> Cheleswer
>
>> -----Original Message-----
>> From: Per Liden
>> Sent: Friday, April 29, 2016 7:06 PM
>> To: Cheleswer Sahu; Mattis Castegren; 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-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
>>>>>>>>
>>>>>>>>
>>>>>>>>