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

[Alex Macdonald]

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