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

[Ken Dobson]

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
>