Review request for JMC-4645: Add size distribution chart to IO Pages

[Joshua Matsuoka]

Hi Ken,

Looks good to me. If there are no further objections I'll push this for you.

Cheers,

- Josh

On Mon, Apr 8, 2019 at 9:31 AM Alex Macdonald <almacdon at redhat.com> wrote:

> On Fri, Apr 5, 2019 at 3:56 PM Ken Dobson <kdobson at redhat.com> wrote:
>
> > And here is an updated webrev that removes the unused imports and should
> > have no excess whitespace.
> >
> > http://cr.openjdk.java.net/~kdobson/sizedistributionchart1/webrev/
> >
>
> Thanks, it looks good to me.
>
>
> >
> > Addressing Alex's question about the distribution, there were 1797 socket
> > reads each of ~1B which adds up to a total of 1.89KiB read.
> >
> > Thanks,
> >
> > Ken Dobson
> >
> > On Fri, Apr 5, 2019 at 3:49 PM Ken Dobson <kdobson at redhat.com> wrote:
> >
> >> Here is the previous messages from this list for some reason they
> haven't
> >> shown up to the others on the list as well as the pipermail archives.
> >>
> >> From: Marcus Hirt <marcus.hirt at datadoghq.com>
> >> Date: Tue, Apr 2, 2019 at 10:55 PM
> >> Subject: Re: Review request for JMC-4645: Add size distribution chart to
> >> IO Pages
> >> To: Ken Dobson <kdobson at redhat.com>
> >> Cc: <jmc-dev at openjdk.java.net>
> >>
> >>
> >> Looks good!
> >>
> >> Kind regards,
> >> Marcus
> >>
> >> On Tue, Apr 2, 2019 at 4:51 PM Ken Dobson <kdobson at redhat.com> wrote:
> >>
> >>> Thanks for the review, here's the new webrev removing unnecessary
> >>> whitespace.
> >>>
> >>> http://cr.openjdk.java.net/~kdobson/sizedistributionchart0/webrev
> >>>
> >>> Ken Dobson
> >>>
> >>> On Mon, Apr 1, 2019 at 9:27 PM Marcus Hirt <marcus.hirt at datadoghq.com>
> >>> wrote:
> >>>
> >>>> Hi Ken,
> >>>>
> >>>> It generally looks good, but:
> >>>>
> >>>>    - When I apply your patch, I see a lot of whitespace line endings.
> >>>>    Whilst we're not enforcing that there is no trailing whitespace,
> it would
> >>>>    be nice to avoid. E.g.
> >>>>
> >>>> [image: image.png]
> >>>>
> >>>>    - Is the reason for adding 64 bytes to the maxValue (if there is
> >>>>    one) cosmetic padding?
> >>>>      sizeMax = sizeMax == null ? UnitLookup.BYTE.quantity(64):
> >>>>    sizeMax.add(UnitLookup.BYTE.quantity(64));
> >>>>
> >>>> Kind regards,
> >>>> Marcus
> >>>>
> >>>> On Fri, Mar 29, 2019 at 4:07 PM Ken Dobson <kdobson at redhat.com>
> wrote:
> >>>>
> >>>>> Hi all,
> >>>>>
> >>>>> Please review this patch I've made that adds a size distribution
> chart
> >>>>> to
> >>>>> the IO Pages.
> >>>>>
> >>>>> Bug: https://bugs.openjdk.java.net/projects/JMC/issues/JMC-4645
> >>>>> Webrev:
> >>>>> http://cr.openjdk.java.net/~kdobson/sizedistributionchart/webrev/
> >>>>>
> >>>>> Thanks,
> >>>>>
> >>>>> Ken Dobson
> >>>>>
> >>>>
> >> On Fri, Apr 5, 2019 at 2:36 PM Alex Macdonald <almacdon at redhat.com>
> >> wrote:
> >>
> >>> Hi Ken,
> >>>
> >>> On Fri, Mar 29, 2019 at 4:08 PM Ken Dobson <kdobson at redhat.com> wrote:
> >>>
> >>>> Hi all,
> >>>>
> >>>> Please review this patch I've made that adds a size distribution chart
> >>>> to
> >>>> the IO Pages.
> >>>>
> >>>> Bug: https://bugs.openjdk.java.net/projects/JMC/issues/JMC-4645
> >>>> Webrev:
> >>>> http://cr.openjdk.java.net/~kdobson/sizedistributionchart/webrev/
> >>>
> >>>
> >>> Overall the code looks good from what I can tell.
> >>>
> >>> Having said that, I just want to confirm what I'm reading off the
> chart,
> >>> let's use this sample screen shot [0] (https://imgur.com/sPo9Xeo) as
> an
> >>> example. The y-axis displays the number of reads/writes, and the x-axis
> >>> displays the size of that read or write? If that's the case I'm a bit
> >>> confused. The blue bar (write) hits 1 on the y axis for 1 read, and is
> >>> displayed at ~6.16KiB on the x axis because that's how much it wrote.
> >>> However, the red bar (read) hits 1797 on the y axis for 1797 reads as
> >>> expected, but why does the x axis display 0 - 256B when there was 1.89
> KiB
> >>> read in total?
> >>>
> >>> A couple of formatting nits:
> >>>
> >>> 1. There are some unused imports in the DataPageToolkit
> >>>
> >>> [...]
> >>> +import org.openjdk.jmc.flightrecorder.ui.PageManager;
> >>> [...]
> >>> +import
> org.openjdk.jmc.flightrecorder.ui.pages.itemhandler.ItemHandlerPage;+import
> org.openjdk.jmc.flightrecorder.ui.pages.itemhandler.ItemHandlerPage.ItemHandlerUiStandIn;
> >>> [...]
> >>>
> >>> 2. Excess white space just before (and inside)
> >>> DataPageToolkit.buildSizeHistogram()
> >>> 3. JDKAttributes, SocketIO & FileIO pages also each have a few
> instances
> >>> of excess/trailing white space
> >>>
> >>>
> >>>>
> >>>> Thanks,
> >>>>
> >>>> Ken Dobson
> >>>>
> >>>
> >>> Cheers,
> >>>
> >>> Alex
> >>>
> >>> [0] https://imgur.com/sPo9Xeo
> >>>
> >>
>