RFR(xxs): 8251257: NMT: jcmd VM.native_memory scale=1 crashes target VM

[David Holmes]

Hi Thomas,

Yes all reviewed.

No need for any follow ups.

Thanks,
David

On 9/08/2020 4:36 pm, Thomas Stüfe wrote:
> 
> 
> On Sun, Aug 9, 2020 at 8:29 AM David Holmes <david.holmes at oracle.com 
> <mailto:david.holmes at oracle.com>> wrote:
> 
>     On 9/08/2020 1:01 am, Thomas Stüfe wrote:
>      > Hi David,
>      >
>      > On Sat, Aug 8, 2020 at 2:32 PM David Holmes
>     <david.holmes at oracle.com <mailto:david.holmes at oracle.com>
>      > <mailto:david.holmes at oracle.com
>     <mailto:david.holmes at oracle.com>>> wrote:
>      >
>      >     Hi Thomas,
>      >
>      >     Wouldn't a scale name of "b" be more appropriate given the
>      >     acceptance of
>      >     "scale=b"? It seems weird to me that scale=1 is even accepted
>     in the
>      >     first place.
>      >
>      >
>      > I am not sure what you refer to, the added case in the switch
>     construct?
> 
>     Yes. I expected to see
> 
>         37   switch(scale) {
>         38     case B: return "B";
>         39     case K: return "KB";
>         40     case M: return "MB";
>         41     case G: return "GB";
>         42   }
> 
>     ...
> 
>      > If yes, this is the string of the unit NMT prints its numbers
>     with. For
>      > byte sizes, we normally omit the unit, so I do it here too.
> 
>     ... but if the convention is to omit the unit for bytes then that is
>     fine. The use of "1" in the switch rather than B made this look even
>     odder to me. And I recognise that for some reason the code uses the
>     alias "1" instead of "B" as the primary way to refer to the "byte"
>     scale.
> 
> 
> Just looked at the history and I added "1" and "b" myself as values in 
> 2018 when I unified scale handling between VM.metaspace and 
> VM.native_memory (JDK-8201572).
> 
> If it bothers you we can change this but I do not see any pressing need; 
> for me "1" works, I think it makes sense.
> 
>      > If you object against the name of the scale as used in the scale
>      > argument, that has been there before and changing it is not
>     subject of
>      > the patch. I just don't want the target VM to crash.
> 
>     Sorry I didn't intend to suggest that you change it, just commenting
>     that it seems quite odd to me and inconsistent with the other scale
>     values.
> 
>      >
>      >     And can you please add some information to the bug report.
>      >
>      >
>      > Done. I thought subject + assert printout would be sufficient,
>     sorry for
>      > the brevity.
> 
>     Thanks.
> 
>     David
> 
> 
> So, reviewed from your side?
> 
> ..Thomas
> 
>      >     Thanks,
>      >     David
>      >
>      >
>      > Thank you, Thomas
>      >
>      >
>      >     On 8/08/2020 4:15 pm, Thomas Stüfe wrote:
>      >      > Hi,
>      >      >
>      >      > may I have a second Review please?
>      >      >
>      >      > All tests at SAP went through successfully, as did the
>     jdk-submit
>      >     tests at
>      >      > Oracle.
>      >      >
>      >      > Thanks!
>      >      >
>      >      > ..Thomas
>      >      >
>      >      > On Thu, Aug 6, 2020 at 5:07 PM Thomas Stüfe
>      >     <thomas.stuefe at gmail.com <mailto:thomas.stuefe at gmail.com>
>     <mailto:thomas.stuefe at gmail.com <mailto:thomas.stuefe at gmail.com>>>
>     wrote:
>      >      >
>      >      >> Hi,
>      >      >>
>      >      >> very simple small fix.
>      >      >>
>      >      >> jcmd VM.native_memory allows for scale=1 to print exact byte
>      >     values, but
>      >      >> crashes when printing.
>      >      >>
>      >      >> JBS: https://bugs.openjdk.java.net/browse/JDK-8251257
>      >      >> webrev:
>      >      >>
>      >
>     http://cr.openjdk.java.net/~stuefe/webrevs/8251257-nmt-scale-1-crash/webrev.00/webrev/
>      >      >>
>      >      >> Thanks, Thomas
>      >      >>
>      >
>