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

[Thomas Stüfe]

On Sun, Aug 9, 2020 at 8:29 AM David Holmes <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>> 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>> 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
> >      >>
> >
>