jmc icon indicating copy to clipboard operation
jmc copied to clipboard

Published 20 hours ago • verified publisher icon openjdk

7894: Provide an alternative Java based flamegraph visualization

Open bric3 opened this issue 2 years ago • 15 comments

This change introduces a possible replacement for the current flamegraph view.

Motivation The current is based on a web view. In the current state,

  • the web view can be slow to render especially when the tree is large.
  • the web view don't feel well integrated, in particular when popups are shown.
  • web view are difficult to work with from a JMC developer perspective.

image

Description Concretely this PR relies on the swing component to render flamegraphs : https://github.com/bric3/fireplace. And plays with the bridge between SWT and AWT via the SWT_AWT class.

As the intent of this view is to eventually replace the current one, the icons are the same.

Since fireplace has no actual release, only snapshots, in order to try this PR, it is necessary to install the snapshot manually before starting the p2 server.

cd releng/third-party

mvn dependency:get -DrepoUrl=https://s01.oss.sonatype.org/content/repositories/snapshots -Dartifact=io.github.bric3.fireplace:fireplace-swing:0.0.1-SNAPSHOT:jar

# if sources are wanted
mvn dependency:get -DrepoUrl=https://s01.oss.sonatype.org/content/repositories/snapshots -Dartifact=io.github.bric3.fireplace:fireplace-swing:0.0.1-SNAPSHOT:jar:sources

# make the p2 site, don't forget the -U
mvn p2:site -U; mvn jetty:run

Outstanding issues / limitation

  • [x] Fireplace has not yet a stable release as some part of its API are a bit rough.
  • [x] Currently the view does not initializes correctly: the swing JScrollPane don't show scroll bars, until the view is resized by the user. I lack the SWT / Swing expertise to understand why at this time. https://github.com/bric3/fireplace/issues/79
  • [x] Fireplace only supports icicle view at this time. https://github.com/bric3/fireplace/issues/22
  • [x] Icons for minimap toggle and zoom reset image
  • [x] Export to image https://github.com/bric3/fireplace/issues/99
  • [x] Export to print ? Can be done at a later time
  • [x] release of fireplace

Progress

  • [x] Commit message must refer to an issue
  • [ ] Change must be properly reviewed (1 review required, with at least 1 Committer)

Issue

  • JMC-7894: Provide an alternative Java based flamegraph visualization

Contributors

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jmc pull/408/head:pull/408
$ git checkout pull/408

Update a local copy of the PR:
$ git checkout pull/408
$ git pull https://git.openjdk.org/jmc pull/408/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 408

View PR using the GUI difftool:
$ git pr show -t 408

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jmc/pull/408.diff

bric3 avatar Jul 06 '22 11:07 bric3

:wave: Welcome back bric3! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

bridgekeeper[bot] avatar Jul 06 '22 11:07 bridgekeeper[bot]

This is great! All we need now is:

  • Support for rendering the thing upside down (flame rather than icicle).
  • Support for hooking in alternative tooltip mechanisms, e.g. perhaps we can do the tooltips in SWT and thereby bypass the limitation of rendering them on the AWT surface?
  • Use an SWT scrolled composite?

thegreystone avatar Jul 06 '22 19:07 thegreystone

Hi thanks

  • Support for rendering the thing upside down (flame rather than icicle).

Need some time around this. Can this come at a later time?

  • Support for hooking in alternative tooltip mechanisms, e.g. perhaps we can do the tooltips in SWT and thereby bypass the limitation of rendering them on the AWT surface?

Actually tooltips are based on Eclipse's JFace ; in a previous implementation I played with SWT's ToolTip but the component doesn't allow multiline or even give some style.

  • Use an SWT scrolled composite?

Let's not rule that out, but that represents a significant work. Currently we should see the fireplace flamegraph as a coherent unit that is composed of a JScrollPane among other swing component.

bric3 avatar Jul 06 '22 23:07 bric3

Actually tooltips are based on Eclipse's JFace.

Nice! So that means we can have similar tooltips to the current ones, but they will not be cut off at the panel boundaries? That's great!

Regarding using an SWT scrolled composite to wrap the component, isn't that the way to do this? I mean, basically setting the width to the width of the parent container, and have the height calculated (and reported) accordingly?

thegreystone avatar Jul 07 '22 16:07 thegreystone

Regarding using an SWT scrolled composite to wrap the component, isn't that the way to do this? I mean, basically setting the width to the width of the parent container, and have the height calculated (and reported) accordingly?

This could have been possible this way, but in order to react correctly to mouse events there's a "tight" integration between listeners. While it could be possible it represents a big rework.

And anyway I spend some time figuring out what was wrong in bric3/fireplace#79, and fixed the issue.

bric3 avatar Jul 08 '22 17:07 bric3

So https://github.com/openjdk/jmc/pull/408/commits/736b3c1f26ee7e8a5e167ca016bd550513d78d96 adds iciclegraph 🍨 (already there) and flamegraph 🔥

image

Also the library now highlight sibling frames (the screenshot above shows several AccessControler.doPrivileged), the comparison is based on AggregatableFrame.

bric3 avatar Jul 28 '22 21:07 bric3

@thegreystone The view is almost on par with the existing, there's no two outstanding issues, icons for some actions, and export actions.

EDIT: Not needed anymore since the swing lib is on maven central now.

Also.

Since fireplace has now no release yet, it's a bit hard to test this PR for a few reasons :

  • The maven p2 plugin replaces -SNAPSHOT by a timestamp that is set when the local p2 site is created, not the timestamp of the snapshot artifact. See https://github.com/reficio/p2-maven-plugin/issues/59
  • Which means that .target files have to be changed locally, after the mvn p2:site -U has been run with the actual filename timestamp e.g. the jar file is releng/third-party/target/repository/plugins/fireplace-swing_0.0.1.20220804110518.jar so the version to update in the .target files is 0.0.1.20220804110518
  • Don't open the .target from the project view, you actually need to open the file using the menu otherwise Eclipse don't see that the .target file has changed so it doesn't use the new snapshot version.

I'd like to release a version soon, but I'd like to make sure most things are alright.

bric3 avatar Jul 28 '22 21:07 bric3

/contributor add bdutheil

bric3 avatar Aug 06 '22 07:08 bric3

@bric3 Contributor Brice Dutheil <[email protected]> successfully added.

openjdk[bot] avatar Aug 06 '22 07:08 openjdk[bot]

If you're the only contributor, you don't need to explicitly add yourself. ;)

thegreystone avatar Aug 06 '22 10:08 thegreystone

/issue JMC-7894

bric3 avatar Sep 08 '22 13:09 bric3

@bric3 The primary solved issue for a PR is set through the PR title. Since the current title does not contain an issue reference, it will now be updated.

openjdk[bot] avatar Sep 08 '22 13:09 openjdk[bot]

Webrevs

mlbridge[bot] avatar Sep 08 '22 13:09 mlbridge[bot]

Do we want to still provide both variants, or simply replace the web based one with the Java based one?

thegreystone avatar Sep 27 '22 13:09 thegreystone

Do we want to still provide both variants, or simply replace the web based one with the Java based one?

No I believe the web variant should be eventually removed. But I think it's better to do that later.

bric3 avatar Sep 27 '22 14:09 bric3

Hi @bric3 - if the svg-files aren't used by the plug-in, you should make a PR in the jmc-graphics project for them instead.

thegreystone avatar Oct 19 '22 17:10 thegreystone

@bric3 this pull request can not 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 flamegraph-swing
git fetch https://git.openjdk.org/jmc master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

openjdk[bot] avatar Feb 17 '23 18:02 openjdk[bot]

Hi Bric! Can you please merge master to resolve the merge conflicts?

thegreystone avatar Feb 17 '23 18:02 thegreystone

@bric3 Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

openjdk[bot] avatar May 24 '23 13:05 openjdk[bot]

Hi @bric3! Seems the pre-submit tests fail on check style stuff, can you please take a look.

thegreystone avatar Jun 07 '23 17:06 thegreystone

@thegreystone Done

bric3 avatar Jun 08 '23 08:06 bric3

Hm, potentially disregard my review comment about the tooltip. It shows up when I launch JMC through Eclipse as an RCP application, but isn't showing up when run a locally built JMC.

aptmac avatar Jun 08 '23 15:06 aptmac

  1. I'd have to take a bit more a look if it's just me, but I don't have any tooltips displayed when hovering the graph lanes.

    Those are showing fine on macOs, regardless it's an eclipse plugin or a standalone app. I don't have a windows under the hod so I don't know.

  2. Maybe there's a way to distinguish this new flame view from the old one? When adding it to the view it might not be intuitive which one to pick, "Flame View vs. Flame Graph View"

    I think the view should replace the old one at some point. Currently I think the view should be tested to see what's missing before make it a "drop-in" replacement.

  3. The save feature is saving an empty file for me.

    Well spotted this only happens on the jpeg format, png works fine.

bric3 avatar Jun 08 '23 19:06 bric3

@aptmac JPG Image saving is fixed.

  1. It looks like you've tagged a rc5 on your repo, should this PR be updated to include that latest version?

    I'd rather not as RC5 has an issue when resizing. And I didn't had the time to fix it properly. I wasn't expecting this issue to require so much involvement. Ideally I would like to fix this at some point before reaching a the next RC and ultimately the stable version.

  2. What is the future for this dependency? Could it be hosted on something like maven central? Just thinking about potential vendor builds & releases of JMC and the validity pulling rc tags from GitHub.

    Currently the libraries are uploaded to central (if they are at least release candidates).

bric3 avatar Jun 08 '23 20:06 bric3

It looks and feels amazing! Well done! I think you should go ahead and just remove the old view. No point in having two of them. Also, perhaps rename the view to just Flame Graph (View is implied)?

thegreystone avatar Jun 08 '23 23:06 thegreystone

It looks and feels amazing! Well done! I think you should go ahead and just remove the old view. No point in having two of them. Also, perhaps rename the view to just Flame Graph (View is implied)?

And, yup, "View" should probably be dropped from more views. ;)

thegreystone avatar Jun 08 '23 23:06 thegreystone

Before replacement I think this view should at least replicate

  • https://github.com/openjdk/jmc/pull/365

bric3 avatar Jun 08 '23 23:06 bric3

Good point. It would make sense to go for feature parity in this PR though - having two of them (one of which is empty on Windows), is confusing.

thegreystone avatar Jun 08 '23 23:06 thegreystone

OK I'll have a stab at this remaining feature ASAP.

bric3 avatar Jun 08 '23 23:06 bric3

Done porting #365, also added the search text field

image

bric3 avatar Jun 09 '23 14:06 bric3