[PATCH] JMC-5128/JMC-5149 - Utilize EventThread field in GC Phase Events

Thu Aug 23 15:30:42 UTC 2018

Hi Marcus,

After our discussions on IRC we decided to go with adding a tooltip over
the disabled options for user clarity. I've gone ahead and added a tooltip
to the disabled legend and form actions, as well as disabled the context
menu actions.

http://cr.openjdk.java.net/~jmatsuoka/JMC-5128/webrev.01/

Cheers,

- Josh

On Mon, Aug 20, 2018 at 12:38 PM, Marcus Hirt <marcus.hirt at oracle.com>
wrote:

> Hi Josh,
>
> Great! Thanks for the revised patch! I am a bit torn regarding the
> disablement
> for two reasons: the first is that there is no feedback as to why it has
> been
> disabled, and the second is that the lane related items on the chart's
> context
> menu still remain available to me, so it is still possible to enter the
> lane
> configuration dialog. How about a quick chat on the IRC to see if we can
> come
> up with an improved solution?
>
> Kind regards,
> Marcus
>
> From: Joshua Matsuoka <jmatsuok at redhat.com>
> Date: Monday, 20 August 2018 at 18:09
> To: Marcus Hirt <marcus.hirt at oracle.com>
> Cc: <jmc-dev at openjdk.java.net>
> Subject: Re: [PATCH] JMC-5128/JMC-5149 - Utilize EventThread field in GC
> Phase Events
>
> Hi Marcus,
>
> I've updated the patch. It now checks if the pause events have thread
> information before proceeding with the rendering, as well as disabling the
> thread activity actions if there is no thread information present in the
> events, as in that situation there is no point to those actions.
>
> Cheers,
>
> - Josh
>
> On Fri, Aug 17, 2018 at 12:49 PM, Marcus Hirt <mailto:
> marcus.hirt at oracle.com> wrote:
> Hi Josh,
>
> In older recordings, the thread information may not be available:
> <-->
> java.lang.NullPointerException
>         at org.openjdk.jmc.flightrecorder.ui.pages.GarbageCollectionsPage$
> GarbageCollectionsUi.buildChart(GarbageCollectionsPage.java:515)
>         at org.openjdk.jmc.flightrecorder.ui.pages.GarbageCollectionsPage$
> GarbageCollectionsUi.lambda$0(GarbageCollectionsPage.java:255)
>         at org.openjdk.jmc.ui.handlers.ActionToolkit$3.run(
> ActionToolkit.java:166)
>         at org.openjdk.jmc.ui.misc.ActionUiToolkit.lambda$4(
> ActionUiToolkit.java:129)
>         at org.eclipse.jface.viewers.CheckboxTableViewer$1.run(
> CheckboxTableViewer.java:212)
>         at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:42)
>         at org.eclipse.ui.internal.JFaceUtil.lambda$0(JFaceUtil.java:44)
> <-->
>
> Ping me on IRC and I can share a representative recording with you!
>
> Kind regards,
> Marcus
>
> From: Joshua Matsuoka <mailto:jmatsuok at redhat.com>
> Date: Tuesday, 14 August 2018 at 17:25
> To: Marcus Hirt <mailto:marcus.hirt at oracle.com>
> Cc: <mailto:jmc-dev at openjdk.java.net>
> Subject: Re: [PATCH] JMC-5128/JMC-5149 - Utilize EventThread field in GC
> Phase Events
>
> Hi Marcus,
>
> Heres an updated patch, I've removed the thread table and made the thread
> activity lane dependent on the table selection.
>
> Cheers,
>
> - Josh
>
> On Mon, Aug 13, 2018 at 4:07 PM, Joshua Matsuoka <mailto:mailto:jmatsuok@
> redhat.com> wrote:
> Hi Marcus,
>
> Sounds good to me! I agree the thread chart seems a little odd looking at
> it again. I'll remove it but keep the activity lane as you suggested.
>
> Would you be opposed to a separate patch adding the thread lane context
> menu entries to the application page? It seems inconsistent with the other
> uses of activity lanes.
>
> Cheers,
>
> - Josh
>
> On Mon, Aug 13, 2018 at 3:59 PM, Marcus Hirt <mailto:mailto:marcus.hirt@
> oracle.com> wrote:
> Hi Joshua,
>
> I agree that the thread field should be added to the phases table. I worry
> about having a threads table in the beginning of the page, as it moves the
> focus from GCs to threads. If you are only interested in thread halts in
> relation to other events in the recording, you should really go to the
> Application page. Then you see all the halts (for example other
> vm-operations), and not only the GC-related ones.
>
> You could keep the Thread Activity lane, but for example make it show
> events for the threads involved in whatever selection you do in the tables
> above (without having an explicit threads table).
>
> What do you think?
>
> Kind regards,
> Marcus
>
> From: Joshua Matsuoka <mailto:mailto:jmatsuok at redhat.com>
> Date: Monday, 13 August 2018 at 20:51
> To: Marcus Hirt <mailto:mailto:marcus.hirt at oracle.com>
> Cc: <mailto:mailto:jmc-dev at openjdk.java.net>
> Subject: Re: [PATCH] JMC-5128/JMC-5149 - Utilize EventThread field in GC
> Phase Events
>
> Hi Marcus,
>
> Thanks for the feedback!
>
> I was following the suggestions in the comments/history on
> https://bugs.openjdk.java.net/browse/JMC-5128 . I think there's value to
> having the thread lane able to be visualized alongside the rest of the GC
> information. It's nice being able to see the thread events/activity align
> with the pause visualization and heap activity. I can also seeing this be
> useful for users who may have custom events defined, where it may be useful
> to see these events alongside the heap/gc behaviour.  I noticed as well
> that the thread activity display on the Java Application page lacks the
> context menu options to display additional types of events in the
> visualization. Perhaps if we feel that these changes are unnecessary, I can
> make an additional patch that could add that functionality to the thread
> lane on the application page. In either case though, we should probably
> still add the event thread to the information displayed for pause events.
>
> Cheers,
>
> - Josh
>
> On Mon, Aug 13, 2018 at 11:56 AM, Marcus Hirt <mailto:mailto:mailto:mailto
> :marcus.hirt at oracle.com> wrote:
> Hi Josh,
>
> Thanks for the suggestion! The normal work process is to focus the UI on
> whatever is of interest, and then just go to the Applications view. That
> way
> we end up not having to replicate that functionality on every page.
>
> Can you tell me a little bit more about your use case and how you see this
> being used?
>
> (Good initiative! I think you're the first non-Oracle employee attempting
> to
> improve on the visualization, so kudos!)
>
> Kind regards,
> Marcus
> On 2018-08-13, 17:41, "jmc-dev on behalf of Joshua Matsuoka" <mailto:
> mailto:mailto:mailto:jmc-dev-bounces at openjdk.java.net on behalf of mailto:
> mailto:mailto:mailto:jmatsuok at redhat.com> wrote:
>
>     Hi,
>
>     This is a patch that addresses JMC-5128 and JMC-5149. Currently the
> event
>     thread field in the GC Phase Events isn't used by the UI. This patch
>     accomplishes the following:
>
>     - Adds this field to the phases table.
>     - Adds a Thread table similar to the Java Application page which
> displays
>     the threads that the phase events occurred on.
>     - Adds a Thread lane to visualize the thread selected by the above
> table.
>
>     This is a screenshot of the updated UI: https://imgur.com/a/CFQz9G6
>
>     Thoughts?
>
>     Cheers,
>
>     - Josh
>
>
>
>
>
>
>
>
>
>
>
>