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
8283775: better dump: VM support for graph querying in debugger with BFS traversal and node filtering #8468
Conversation
👋 Welcome back eme64! A progress list of the required criteria for merging this PR into |
Webrevs
|
I tried the functionality and found it useful, thanks! Just a few comments:
|
In conversation with @TobiHartmann we decided to make it a member function of |
Thanks for looking at it, @robcasloz !
I like the idea of making a
Yes, it would be nice to avoid code duplication. The question is if people will be ok with having their tools replaced. |
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.
Nice.
@eme64 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 411 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 |
Feedback from @chhagedorn : refactor |
@eme64 this pull request can not be integrated into git checkout JDK-8283775
git fetch https://git.openjdk.java.net/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push |
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.
These are cool new dumping features! I especially like the coloring and the filtering which follow the coloring and filtering of IGV. The implementation looks good apart from some minor code styles things. Thanks for addressing all the offline update suggestions!
@vnkozlov should also review it again as his review applies to an old state which is now quite different.
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.
Looks good, thanks for doing the updates.
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.
Update 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.
Looks great!
Thank you @TobiHartmann Co-authored-by: Tobias Hartmann <tobias.hartmann@oracle.com>
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 adding this functionality, Emanuel! I tried it out and works fine for my regular use cases. The code itself looks good, too. I have a few minor comments, please take them as suggestions only.
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 comments, looks good!
/integrate |
Going to push as commit 33ed036.
Your commit was automatically rebased without conflicts. |
What this gives you for the debugger
Some usecases
dump
Description
I implemented VM support for BFS traversal of IR nodes, where one can filter for input/output edges, and the node-type (control / data / memory). If one specifies the
target
node, we find a shortest path betweenthis
andtarget
. With theoptions
string one can easily select which node types to visit (cdmxo
) and which to include only in the boundary (CDMXO
). To find all paths between two nodes, include the letterA
in the options string.void Node::dump_bfs(const int max_distance, Node* target, char const* options)
To get familiar with the many options, run this to get help:
find_node(0)->dump_bfs(0,0,"h")
While a sufficient summary and description is in the comments above the function definition, I want to explain some useful use-cases here.
Please let me know if you would find this helpful, or if you have any feedback to improve it.
Thanks, Emanuel
PS: I do plan to refactor the
dump
code innode.cpp
to use my new infrastructure. I will also removeNode::related
anddump_related,
since it has not been properly extended and maintained. But that refactoring would risk messing with tools that depend ondump
, which I would like to avoid for now, and do that in a second step.Better dump()
The function is similar to
dump()
, we can also follow input / output edges up to a certain distance. There are a few improvements/added features:+
or output-
edges, or both+-
.type->category()
help one limit traversal to control, data or memory flow. The categories are the same as in IGV.dcmxo
for visit filter, andDCMXO
for boundary filter.#
in the options string! To more easily trace chains of nodes, I highlight the node idx of all nodes that are displayed in their respective colors.@
in options string.B
in options string.S
to the option string!Example (BFS inputs):
Example (BFS control inputs):
We see the control flow of a strip mined loop.
Experiment (BFS only data, but display all nodes on boundary)
We see the dependent output nodes of the data-phi 102, we see that a SafePoint and the Return depend on it. Here colors are really helpful, as it makes it easy to separate the data-nodes (blue) from the boundary-nodes (other colors).
Example with Mach nodes:
And the query on the old nodes:
Exploring loop body
When I find myself in a loop, I try to localize the loop head and end, and map out at least one path between them.
loop_end->print_bfs(20, loop_head, "c+")
This provides us with a shortest control path, given this path has a distance of at most 20.
Example (shortest path over control nodes):
Once we see this single path in the loop, we may want to see more of the body. For this, we can run an
all paths
query, with the additional characterA
in the options string. We see all nodes that lay on a path between the start and target node, with at most the specified path length.Example (all paths between two nodes):
We see there are multiple paths. We can quickly see that there are paths with length 5 (
apd = 5
): the control flow, but also the data flow for the loop-back condition. We also see some paths with length 8, which feed into178 If
and148 Rangecheck
. Node that the distanced
is the distance to the start node741 CountedLoopEnd
. The all paths distanceapd
computes the sum of the shortest path from the current node to the start plus the shortest path to the target node. Thus, we can easily compute the distance to the target node withapd - d
.An alternative to detect loops quickly, is running an all paths query from a node to itself:
Example (loop detection with all paths):
We get the loop control, plus the loop-back
190 IfTrue
.Example (loop detection with all paths for phi):
Color examples
Colors are especially useful to see chains between nodes (options character
#
).The input and output node idx are also colored if the node is displayed somewhere in the list. This should help you find chains of nodes.
Tip: it can be worth it to configure the colors of your terminal to be more appealing.
Example (find control dependency of data node):
We see data nodes in blue, and find a
SafePoint
in red and theReturn
in yellow.Example (find memory dependency of data node):
Example (loop detection):
We find the control and some data loop paths.
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/8468/head:pull/8468
$ git checkout pull/8468
Update a local copy of the PR:
$ git checkout pull/8468
$ git pull https://git.openjdk.java.net/jdk pull/8468/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 8468
View PR using the GUI difftool:
$ git pr show -t 8468
Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/8468.diff