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

[Ken Dobson]

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/

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
>>
>