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

8264795: IGV: Upgrade NetBeans platform #3361

Closed
wants to merge 48 commits into from

Conversation

robcasloz
Copy link
Contributor

@robcasloz robcasloz commented Apr 6, 2021

This change upgrades the NetBeans platform on which IGV is based to its latest version (12.3) and switches IGV's build system from Ant to Maven. The upgrade introduces support for a wide range of JDK versions (from 8 to 15, the latest version supported by NetBeans 12.3), and the switch from Ant to Maven makes the IGV build simpler, faster (first-time build is approximately 5x faster), and more stable (all dependencies are fetched directly from the Maven central repository).

The change also fixes broken unit tests in the Data module and runs them by default when building.

Testing

Regression-tested the following use cases manually on all combinations of (Linux, Windows, macOS) and (JDK 8, JDK 11, JDK 15):

  • build
  • open graph file (small.xml in test-input.zip)
  • import graphs via network (localhost)
  • expand groups in outline
  • open a graph
  • open a clone
  • zoom in and out
  • search a node
  • apply filters
  • extract a node
  • show all nodes
  • select nodes corresponding to a bytecode
  • view control flow
  • select nodes corresponding to a basic block
  • cluster nodes
  • show satellite view
  • compute the difference of two graphs
  • change node text
  • remove a group
  • save groups into a file
  • open a large graph file (large.xml in test-input.zip)
  • open a large graph ("After Escape Analysis" in large.xml)

Thanks to Vladimir Ivanov for helping with testing on macOS.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3361/head:pull/3361
$ git checkout pull/3361

Update a local copy of the PR:
$ git checkout pull/3361
$ git pull https://git.openjdk.java.net/jdk pull/3361/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3361

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3361.diff

@openjdk openjdk bot removed the build build-dev@openjdk.org label Apr 6, 2021
@openjdk
Copy link

openjdk bot commented Apr 6, 2021

@robcasloz
The build label was successfully removed.

@robcasloz robcasloz marked this pull request as ready for review April 6, 2021 19:00
@openjdk openjdk bot added the rfr Pull request is ready for review label Apr 6, 2021
@mlbridge
Copy link

mlbridge bot commented Apr 6, 2021

Webrevs

Copy link
Contributor

@vnkozlov vnkozlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!
After this is pushed consider to update wiki page too: https://wiki.openjdk.java.net/display/HotSpot/IdealGraphVisualizer

@openjdk
Copy link

openjdk bot commented Apr 6, 2021

@robcasloz This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8264795: IGV: Upgrade NetBeans platform

Upgrade IGV's underlying NetBeans platform to version 12.3, switch build system
from Ant to Maven, and fix broken unit tests in Data module.

Reviewed-by: kvn, chagedorn, neliasso, xliu

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 136 new commits pushed to the master branch:

  • f2f7aa3: 8262291: Refactor reserve_memory_special_huge_tlbfs
  • 008fc75: 8264224: Add macosx-aarch64 to Oracle build configurations
  • f4e6395: 8264190: Harden TLS interop tests
  • 18bec9c: 8265084: [BACKOUT] 8264954: unified handling for VectorMask object re-materialization during de-optimization
  • 9dd9625: 8263763: Synthetic constructor parameters of enum are not considered for annotation indices
  • 1ee80e0: 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding
  • d84a7e5: 8264124: Update MXBean specification and implementation to extend mapping of CompositeType to records
  • 714ae54: 8258788: incorrect response to change in window insets [lanai]
  • f479437: 8265082: test/hotspot/jtreg/gc/g1/TestG1SkipCompaction.java fails validate-source
  • 27f4b27: 8264623: Change to Xcode 12.4 for building on Macos at Oracle
  • ... and 126 more: https://git.openjdk.java.net/jdk/compare/011f6d13ab306581f595d6743ca0ff6dfbf95932...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Apr 6, 2021
@robcasloz
Copy link
Contributor Author

Great work!
After this is pushed consider to update wiki page too: https://wiki.openjdk.java.net/display/HotSpot/IdealGraphVisualizer

Thanks for reviewing, Vladimir! Yes, I plan to do that.

Copy link
Member

@chhagedorn chhagedorn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice work!

Copy link

@neliasso neliasso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tested IGV on a High-DPI screen (4K). The screens are attached to the bug-report.

Compared to baseline the new version improved DPI scaling on both JDK8 and JDK15. Some elements still doesn't scale.

Remaining issues identified:

  1. The default zoom level is to small
  2. The filter view has broken scaling - the font scales but not the line height.
  3. The buttons above the graph view and file list doesn't scale - they are still tiny.
  4. The splash screen doesn't scale

Still this is a big improvement and the remaining issues can be solved in separate PRs.

Looks good - Reviewed!

@robcasloz
Copy link
Contributor Author

Very nice work!

Thanks for reviewing, Christian!

@robcasloz
Copy link
Contributor Author

I have tested IGV on a High-DPI screen (4K). The screens are attached to the bug-report.

Compared to baseline the new version improved DPI scaling on both JDK8 and JDK15. Some elements still doesn't scale.

Remaining issues identified:

1. The default zoom level is to small

2. The filter view has broken scaling - the font scales but not the line height.

3. The buttons above the graph view and file list doesn't scale - they are still tiny.

4. The splash screen doesn't scale

Still this is a big improvement and the remaining issues can be solved in separate PRs.

Looks good - Reviewed!

Thanks for reviewing, Nils! I will create a separate RFE for DPI scaling issues.

I also added a clarification to the README file (e84d171) based on our offline discussion.

Copy link
Member

@navyxliu navyxliu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for modernizing IGV. I manage to import this project to IntelliJ. The process is hassle-free!

I also ran using jdk8/jdk11 on macOS. I haven't identified any problem so far.

src/utils/IdealGraphVisualizer/Filter/pom.xml Show resolved Hide resolved
@y1yang0
Copy link
Member

y1yang0 commented Apr 11, 2021

Nice work. It works on Windows home pc. 🚀

@robcasloz
Copy link
Contributor Author

Thank you for modernizing IGV. I manage to import this project to IntelliJ. The process is hassle-free!

I also ran using jdk8/jdk11 on macOS. I haven't identified any problem so far.

Thanks for checking, Xin!

@robcasloz
Copy link
Contributor Author

Nice work. It works on Windows home pc. rocket

Thanks for checking, Yang!

@robcasloz
Copy link
Contributor Author

/summary
Upgrade IGV's underlying NetBeans platform to version 12.3, switch build system
from Ant to Maven, and fix broken unit tests in Data module.

@openjdk
Copy link

openjdk bot commented Apr 13, 2021

@robcasloz Setting summary to:

Upgrade IGV's underlying NetBeans platform to version 12.3, switch build system
from Ant to Maven, and fix broken unit tests in Data module.

@robcasloz
Copy link
Contributor Author

/integrate

@openjdk openjdk bot closed this Apr 13, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 13, 2021
@openjdk
Copy link

openjdk bot commented Apr 13, 2021

@robcasloz Since your change was applied there have been 136 commits pushed to the master branch:

  • f2f7aa3: 8262291: Refactor reserve_memory_special_huge_tlbfs
  • 008fc75: 8264224: Add macosx-aarch64 to Oracle build configurations
  • f4e6395: 8264190: Harden TLS interop tests
  • 18bec9c: 8265084: [BACKOUT] 8264954: unified handling for VectorMask object re-materialization during de-optimization
  • 9dd9625: 8263763: Synthetic constructor parameters of enum are not considered for annotation indices
  • 1ee80e0: 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding
  • d84a7e5: 8264124: Update MXBean specification and implementation to extend mapping of CompositeType to records
  • 714ae54: 8258788: incorrect response to change in window insets [lanai]
  • f479437: 8265082: test/hotspot/jtreg/gc/g1/TestG1SkipCompaction.java fails validate-source
  • 27f4b27: 8264623: Change to Xcode 12.4 for building on Macos at Oracle
  • ... and 126 more: https://git.openjdk.java.net/jdk/compare/011f6d13ab306581f595d6743ca0ff6dfbf95932...master

Your commit was automatically rebased without conflicts.

Pushed as commit 954b9a1.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-compiler hotspot-compiler-dev@openjdk.org integrated Pull request has been integrated
6 participants