-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
8261336: IGV: enhance default filters #2499
Conversation
👋 Welcome back rcastanedalo! A progress list of the required criteria for merging this PR into |
@robcasloz The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice!
@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:
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 106 new commits pushed to the
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 |
Thanks Vladimir! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise, very nice improvement! I applied the patch and played around with it in the IGV. Everything seems to work as expected. I like the new default filters and the new colors for the new categories. That makes it much more useful.
...ualizer/ServerCompiler/src/com/sun/hotspot/igv/servercompiler/filters/onlyControlFlow.filter
Outdated
Show resolved
Hide resolved
Thanks for reviewing Christian! I will have a look at your comments on Monday. |
/contributor add @chhagedorn |
@robcasloz |
I have addressed them now, please re-review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing my suggestions. I tested it again - looks good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome. Thanks a lot for taking the time to improve the IGV. Looks good to me!
Thanks Christian and Tobias for reviewing! |
/summary |
@robcasloz Setting summary to:
|
/integrate |
@robcasloz Since your change was applied there have been 106 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit 16bd7d3. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Redesign the filters shown by default in the "Filters" window:
Add filters to color the graph by node category and execution frequency (if applicable), and to hide subgraphs or edges only by category. The category of a node can be one of {
data
,memory
,control
,mixed
,other
}, and is solely determined by its type.mixed
nodes are those with a tuple type that has different categories, such asCallStaticJavaNode
. The category of an edge is that of its source node.Instrument C2 to include the category and estimated execution frequency (if available) of each node in the graph dumps produced by
-XX:PrintIdealGraphLevel=N
(only in debug builds).Remove filters which depend on properties never emitted by C2 (e.g. 'Remove State') or which appear to be unused ('C2 Matcher Flags Coloring' and 'C2 Register Coloring'). Also remove the subsumed 'C2 Basic Coloring' filter.
Merge 'C2 Remove Filter' and 'C2 Structural' into a single filter with a clearer name ('Simplify graph').
Screenshots
"Filters" window before (left) and after (right) the proposed change:
Default color scheme before (left) and after (right) the proposed change:
Examples of the new 'Color by execution frequency' filter:
Example of the new 'Hide X subgraph'' filters, where only the data subgraph is shown:
Example of the new 'Hide X edges' filters, where all nodes remain in their position but only the memory edges are shown:
Tested IGV manually on a few graphs. Tested C2 instrumentation by running
hs-tier1
with-Xbatch -XX:PrintIdealGraphLevel=3 -XX:PrintIdealGraphFile=graph.xml
on windows-x64, linux-x64, linux-aarch64, and macosx-x64 (all debug).Progress
Issue
Reviewers
Contributors
<chagedorn@openjdk.org>
Download
$ git fetch https://git.openjdk.java.net/jdk pull/2499/head:pull/2499
$ git checkout pull/2499