Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

6364: Improvements to the Thread Graph #27

Open
wants to merge 5 commits into
base: master
from

Conversation

@aptmac
Copy link
Contributor

aptmac commented Jan 8, 2020

This patch addresses JMC-6364 [0], the epic for tracking Improvements to the JFR Thread Graph. This RFR is also a follow-up to an RFC [1] that was posted to the jmc-dev list and incorporates the feedback that was brought up during discussion.

The new design and features was imagined in collaboration with our UX team, and the implementation has been a joint effort between myself and Jessye (@jessyec-s) - we had been originally collaborating on a fork of the old (unofficial) JMC GitHub repo. I've folded all the commits down into one because the older commits were merged using PRs with a similar format (e.g., jmc/pull/23) to this repo, so to avoid accidental noise (sorry about that) on other PRs I've just folded them away. If anyone is interested in the individual commits that got to this point, I've kept them around in the old repo [2].

This PR aims to improve the usability and functionality of the JFR Threads Page by extending and enhancing existing classes. In an attempt to not clutter this PR with lots of images and explanatory text, I've created a gist [3] that provides information and images/gifs of the new components and intended functionality.

Gist: https://gist.github.com/aptmac/61808c89bfcf1888080884fc8a61bd36

Summary of changes:

  • Introduction of canvases to display the thread names (left) and timeline (bottom)
  • Scrolled composites to house the text and chart canvases, for vertical scrolling of the chart area
  • Timeline canvas (bottom) is draggable for panning the chart
  • Current threads table has been re-located to a popup table
  • New filter bar (top) which alters the chart view based on time ranges and desired visible activity lanes
  • New display bar (right) which houses the zoom mechanics
  • New zoom pan component for easily navigating the chart
  • Introduction of a colour palette to increase contrast between neighbouring activity lanes
  • Controls to change the height of the thread lanes
  • updated and new uitests

Before:
before

After:
after

Let me know if you have any questions about the design & functionality.

[0] https://bugs.openjdk.java.net/browse/JMC-6364
[1] http://mail.openjdk.java.net/pipermail/jmc-dev/2019-October/001454.html
[2] https://github.com/aptmac/jmc-old/commits/jfr-threads-page
[3] https://gist.github.com/aptmac/61808c89bfcf1888080884fc8a61bd36


Progress

  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JMC-6364: Improvements to the Thread Graph

Download

$ git fetch https://git.openjdk.java.net/jmc pull/27/head:pull/27
$ git checkout pull/27

@bridgekeeper
Copy link

bridgekeeper bot commented Jan 8, 2020

👋 Welcome back aptmac! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request (refresh this page to view it).

@jiekang
Copy link
Contributor

jiekang commented Jan 10, 2020

jcheck is requesting:

s/JMC-6364/6364

@aptmac aptmac force-pushed the aptmac:jfr-threads-page-folded branch from cda08a5 to 5dc0e35 Jan 10, 2020
@aptmac aptmac changed the title JMC-6364: Improvements to the Thread Graph 6364: Improvements to the Thread Graph Jan 10, 2020
@openjdk openjdk bot added the rfr label Jan 10, 2020
@mlbridge
Copy link

mlbridge bot commented Jan 10, 2020

Webrevs

@aptmac
Copy link
Contributor Author

aptmac commented Jan 10, 2020

jcheck is requesting:

s/JMC-6364/6364

Neat, thanks for the heads up. I changed the commit description and force pushed them back here, but it looks like once a PR has been made with the invalid ID format it's the PR name change that gets jcheck to pass. This automation is nice, the mvn spotless:apply was especially nice to have.

@Gunde
Copy link

Gunde commented Jan 20, 2020

I actually liked the old behaviour of being able to see all thread states on one screen (given that you had sufficient pixels to display them). It gave a more helpful overview of the application state during the recording. I.e. the old view had a minimum lane height of 1 pixel. To do the same thing here you'd have to collapse the table, since 1 pixel isn't enough to render thread names.

The selection highlighter also seems to have taken a significant performance hit, so it's a bit harder to drag and select a region of threads.

Unless I'm missing something it also appears that I can't sort the thread table, and I'm uncertain what order the threads are shown in now.

I'm also a bit confused by why the scroll bar on the right hand side scrolls horizontally when it's rendered vertically, and the component now doesn't zoom through time with the scroll wheel, like all other pages do?

It would also be nice if the "Thread State Selection" toolbar button could do more of the "Edit Thread Activity Lanes" dialog, since they're doing very similar things?

@aptmac
Copy link
Contributor Author

aptmac commented Jan 23, 2020

I actually liked the old behaviour of being able to see all thread states on one screen (given that you had sufficient pixels to display them). It gave a more helpful overview of the application state during the recording. I.e. the old view had a minimum lane height of 1 pixel. To do the same thing here you'd have to collapse the table, since 1 pixel isn't enough to render thread names.

Fair enough, or perhaps allowing the chart lanes to shrink to that size, but have the text lanes remain at a size that is readable? e.g., having the text lanes hit a min height of ~13px, so there will only be a difference in lane height for 1-12px, and everything above that they re-sync in terms of height?

The selection highlighter also seems to have taken a significant performance hit, so it's a bit harder to drag and select a region of threads.

Unless I'm missing something it also appears that I can't sort the thread table, and I'm uncertain what order the threads are shown in now.

It should be the same order as they are now, the current table has been moved to be like a modal instead, it's available to open via a button near the "edit thread activity lanes" button in the top-right of the page. It's the same table, so the existing sorting mechanics should apply.

I'm also a bit confused by why the scroll bar on the right hand side scrolls horizontally when it's rendered vertically,

Are you referring to the SWT spinner? That handles zooming in/out of the chart, and the scrollbar to the right of the chart (should) scroll vertically.

and the component now doesn't zoom through time with the scroll wheel, like all other pages do?

This has to do more with what the expected behaviour should be on a page that has scrollable content. In the cases of the other pages, the charts/graphs don't scroll vertically, so having JMC-specific defined controls with the scrollwheel to zoom should be fine. However, in many other applications where pages have scrollable content (e.g., internet browsers, photoshop, music editing software, etc.) the scrollwheel is reserved for vertically navigating the content.

It would also be nice if the "Thread State Selection" toolbar button could do more of the "Edit Thread Activity Lanes" dialog, since they're doing very similar things?

I could take a look into that, the concern was that it's quite a bit of information/controls to fit into a dropdown of limited space without having it overlap too much of the chart content.

@openjdk openjdk bot added the outdated label Mar 9, 2020
@openjdk
Copy link

openjdk bot commented Mar 26, 2020

@aptmac this pull request can no longer be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout jfr-threads-page-folded
git fetch https://git.openjdk.java.net/jmc master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push
@openjdk openjdk bot added the merge-conflict label Mar 26, 2020
@aptmac aptmac force-pushed the aptmac:jfr-threads-page-folded branch from 5dc0e35 to 08a7b1a Apr 28, 2020
@openjdk openjdk bot removed the merge-conflict label Apr 28, 2020
@thegreystone
Copy link
Collaborator

thegreystone commented Apr 29, 2020

Hi @aptmac - could you update this PR, please? Thinking of taking a closer look at it during the jmc dev hangout tonight. :)

@aptmac
Copy link
Contributor Author

aptmac commented Apr 29, 2020

Hi @thegreystone, I rebased the branch yesterday to be on top of master, and included a new commit based on the feedback that makes the dropdown lane filter act like the Edit Lanes Dialog (screenshot below). Does the outdated tag automatically remove itself what some criteria is met?

Edit: I also see that the checks failed, it looks like there's something wrong with the builds at the moment, I've been seeing similar errors on my travis builds as well

2020-04-27-171139_1177x829_scrot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.