RFR(xxs): 8251257: NMT: jcmd VM.native_memory scale=1 crashes target VM
Thomas Stüfe
thomas.stuefe at gmail.com
Sun Aug 9 07:04:08 UTC 2020
Thank you David.
On Sun, Aug 9, 2020 at 8:45 AM David Holmes <david.holmes at oracle.com> wrote:
> 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
> > > >>
> > >
> >
>
More information about the hotspot-runtime-dev
mailing list