RFR: JMC-5327: Using HdrHistograms to visualize latencies

[Alex Macdonald]

On Thu, May 23, 2019 at 7:06 PM Elliott Baron <ebaron at redhat.com> wrote:

> Hi,
>
> Sorry for the delay. Thank you both for your reviews!
>
> On 2019-05-02 12:55 p.m., Alex Macdonald wrote:
> > A handful of minor formatting nits:
> >
> > - Messages.properties (x3)
> > - - the DurationPercentileTable_PERCENTILE_COL_NAME should come after
> > DUMP_RECORDING_* for alphabetical order I believe
>
> Should be fixed now. I took this into account for the new keys added in
> this revision too.
>
> > - There are some cases of extra whitespace, I went through hg diff and
> > counted the following number of occurrences:
> > - - DurationHdrHistogram (8)
> > - - DurationPercentileTable (38)
> > - - FileIOPage (5)
> > - - SocketIOPage (6)
> > - - IOPageTest (5)
> > - - IOPageTestBase (4)
> > - - SocketIOPageTest(5)
>
> This revised patch should not have any trailing whitespace.
>
> > I noticed that when resizing the page, the durations table stays static
> > whereas the other tables seems to adjust (gif) [3]. This only seems to
> > cause an issue when my JMC window gets resized to just under 50% on my
> > monitor width, and the chart will get hidden. In the gif my JMC window
> > is around half my monitor width (of 1920), and with the JVM
> > Browser/Outline window open then I don't have much more room to shrink
> > the application before the chart disappears. I was just curious if the
> > table could adjust to resizing like the other tables.
>
> This was a bit tricky due to the table sitting side-by-side with the
> chart. I used the (unused?) SimpleLayout class within JMC to define
> weights to keep the percentage of the width occupied by the chart and
> table constant, even while resizing.
>

Thanks for taking a look into this! All of my comments have been addressed.


>
> > Also not caused by this patch (but thought it'd be interesting to
> > share), I'm seeing a bug where my chart text gets really large [4] when
> > I run JMC locally and not as an RCP application (I recall seeing a JIRA
> > bug for this before, but cannot find it), and my durations tab will only
> > show the table (and not even the whole thing) [5].
> I am still seeing this as well.
>
> On 2019-05-02 3:02 p.m., Marcus Hirt wrote:
> > This is a great start! Aside from the comments already provided by Alex,
> > here is some feedback:
> >
> > * It would be great to be able to select a percentile and be able to do
> > set as focused selection, and add the events for that percentile to the
> > focused selection.
>
> This should be working now. When setting a table row as the focused
> selection, all events with duration at least as long as the percentile
> values in that row will be selected.
>
> e.g. Setting the 99th percentile row as the focused selection, with read
> duration of 10ms and write duration of 50ms, will only show read events
>  >= 10ms and write events >= 50ms.
>
> > * The current UI splits read and writes in two different lanes, perhaps
> > that would make sense here too, also for selection purposes. And just as
> > with the lanes, only show when there is available data.
>
> With this revised patch, when there are no matching read or write
> events, the corresponding columns will not be shown. There is a slight
> issue when columns are hidden and then reappear, the last column is too
> large and needs be resized manually. I can reproduce this in other
> tables by hiding two columns, and restoring them. I'm not sure of the
> cause.
>
> > * Perhaps the counts shown should be the number of events in the
> > percentile or above, as those would be the number of events selected -
> > i.e. if you select events in the 99.9 percentile, you probably don't
> > want all of the events up to and including the ones in the 99.9th
> > percentile, but rather the outliers, the ones in 99.9 and above.
>
> Makes sense to me, the event counts now show events with duration
> greater or equal to the value in that row.
>
> > * It would be cool to add a normalized backdrop in the tables to get a
> > rough visual representation of how many of the events are actually in
> > the different buckets.
>
> I have added this as well.
>
> Here is a revised webrev that should address all of the feedback
> received so far.
>
> Bug: https://bugs.openjdk.java.net/browse/JMC-5327
> Webrev: http://cr.openjdk.java.net/~ebaron/JMC-5327/webrev.0/
> Preview: https://imgur.com/a/oqX0fai
>
> Thanks,
> Elliott
>