-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Call tree analysis with graph databases (e.g. Neo4j) v2 #3128
Conversation
Hi @galderz I have been experimenting with this, and I noticed the following. When navigating the txt call tree it's trivial to follow the call chain back to the root, while in the neo4j graph it is not.
This however ends up being extremely heavy, and the reason is that in neo4j we model the call tree as a graph and not as a tree. So to illustrate this we get the following graphs: Graph corresponding to the txt representationGraph corresponding to the neo4j representationIn the former case it's trivial to find the callers of C (D1, B1, and A), while in the latter case there is not enough info to know how we reached C by just walking up from C. As real graphs are significantly bigger and more complex than this example finding the path between A and C becomes increasingly heavy, and thus impractical. The trivial approach to solve this would be to replicate the txt representation and have a node per line, this however doesn't allow one to visit a method and see all the possible incoming and outgoing edges to and from that method. Instead they will get multiple nodes which might not be ideal for some queries/use-cases. WDYT? |
In general, I really like this idea. I believe that having both automated and visual way of processing call trees, points-to analysis results, class initialization chains etc is the way to go. One possible problem I can see is having neo4j specific code in native image codebase. One way to avoid this would be to have a unified tool-friendly output format for all relevant debug data, which would be precisely specified and documented. What do you think about it, @christianwimmer ? |
It just generates a Cypher script, right? And Cypher is standardised by now: https://en.wikipedia.org/wiki/Cypher_(query_language) |
@zakkak Aren't W, X and Z callers too, which you are likely to miss, due to the distributed nature of the txt representation? Probably I'm misunderstanding something. Clarification would be appreciated. |
B, W, X and Z are all indeed callers of D.
In the distributed representation the info is still there but you get multiple graphs instead of a single one for the same method. If for example you want to find the callers of D you will get the following graphs as a result:
HTH |
@zakkak Interesting, then I misunderstood the format all the time (that's for two days ;-). Could you share two txt snippets, one demonstrating the situation from your example, and one where the calls from W, X and Z actually (might) call C as well? If this is going to far off topic feel free to recommend a different channel |
Thanks for the PR, using a generic graph database is certainly interesting. There is also the GraalVM specific "Dashboard" dump format for the points-to information, see Line 117 in a52eb97
Regarding the choice of graph database: It should not be specific to just Neo4J. Instead, there should be a separate option to enable that dumping, which takes a specific database name as the parameter. So you would say something like
|
Apparently I had too much faith in the static analysis and it looks like I was wrong about being able to walk up the tree in the text representation (I didn't know how to read the call-tree properly). So it looks like my suggestion is void :(. @schauder thanks for pushing me in the right direction :) |
For the record. However further inspection (now that I know what to look for) suggests that I was wrong. The text below is part of the calltree that leads to
If we take the
Which indicates that there are at least 9 call chains leading to |
I'm very happy to see that people in Java Community see the power of graph visualization and analysis, as I have been in the field for more than 10 years, advocating it for many different use cases. I have a general concern, however, about this PR containing vendor-specific codes. More specifically, the PR produces not only data files (as CSV files) but also a loader script for a specific database: Cypher for Neo4J. The CSV data files are good. CSV is a popular way for exporting/importing data that are widely supported by virtually every graph database or library. The Cypher loading script, however, is very vendor-specific. Cypher is a proprietary language for Neo4J and does not work for other graph databases -- AWS Neptune, TigerGraph, Oracle Property Graph, Stellagraph, dGraph,, etc ... Note that those other systems can indeed load the CSV files and execute the example graph queries without problems, but only with its own different syntax. Although @christianwimmer proposed to have a abstraction layer for vendor-specific code, I am not sure that is the best way to resolve this. Sure, I can write a code to generate SQL*Loader code to dump the data into Oracle Property Graph. Somebody in TigerGraph can come and write GSQL script to do the same. Neptune person can have another go .... But is this really worth to keep all of them in GraalVM? What if some vendor goes out of business in future? What if some vendors make changes to their query language? I think it best that the GraalVM only produces CSV datafiles. To be more precise, we just remove "printCypher" part of this PR. Every graph database vendor (or it supporters) can write up its own blog articles on how their system can load up call-tree CSV files, run graph queries and do visualization with their own syntax. What do you think? |
Hey @hongsup2 even though I am a Neo4j employee I would agree with your statement and I would like to add a couple of thoughts: The CSV files are versatile and highly useful, not only for the teams at RedHat, but also on Neo4j and VMWare working with similar tasks for Spring Data. What @galderz managed to create with the loader script is great and of course not in vain. The loader script can of course be somewhere referenced ("one way of doing it…"). In addition to the vendor specifics, a different analysis might need the graph in a different shape, as already has been pointed out here. |
Well those are arbitrary names anyway, also for our product. |
Thanks everyone for the input. It seems to me the best option would be to just produce generic CSV. Then, as part of documentation, show how one would do import it to say, Neo4j. My only doubt is then, where the logic to do this should live...
@christianwimmer Any reason to keep the call tree analysis output and logic separate from the dashboard that you mention? Should be used one instead of the other? It's the first time I've heard of the dashboard and json/bgv outputs available there. Where should the code that produces the CSV files be generated from? From inside the |
486a119
to
5c63f6c
Compare
That depends on the level of detail that you want your output to have. If you are only interested in call edges, then The dashboard output provides much more detailed information about why a certain call graph edge is present. If you feel like you are going in that direction and level of detail, then extending the dashboard output is better. The Dashboard itself is linked from https://www.graalvm.org/docs/tools/ if you want to try it. Especially the static analysis output is still quite experimental though. |
8817f05
to
0b81c76
Compare
Played around with the dashboard but it didn't feel the right place. Instead I've moved the code into The next step would be to document this and show how to use it with an example graph db, e.g. neo4j. Where would that go? |
c9f70ba
to
140920a
Compare
@zakkak the reports documentation has some details that should clarify the confusion around the call tree textual representation, especially the logic behind the use of |
@cstancu @christianwimmer Can this PR be integrated unless you have specific objectives on where to put documentation about using the CSV files? As said above, this documentation is mostly graph tool specific (e.g. how to combine get the csv files imported into neo4j). Is there place for that? @michael-simons do you have any ideas? |
@cstancu Is |
I've added a commit to add some documentation to |
Just a reminder about this - I am once again looking into call graphs, and it's really hard to crack with the current text file format as I'm looking into non-trivial applications: just the section listing all possible code places overriding Clearly I have a problem of "too large" - but that's what I'm aiming to fix: as it often happens, a minor change in our code or a minor upgrade in GraalVM can suddenly trigger a significant additional overhead, and when this happens the tooling we have to track it down makes it tricky. I'm rebuilding a custom fork to include this, but would realy love if this could be unnecessary. |
I've been using this all day yesterday and it's amazing. It would have been far better to keep the ability to generate cypher scripts though - I needed to re-import data multiple times to track a complex issue, and this idea of having only CSVs is very time consuming. Could we at least produce reports with a constant name, or have an option for that? Currently the produced filenames encode both the base name of the built binary and timestamps - scripting this to get in an efficient workspace is very inconvenient. |
} | ||
|
||
ReportUtils.report("call tree for vm entry point", path + File.separatorChar + "reports", "csv_call_tree_vm_" + reportName, "csv", | ||
CallTreePrinter::printVMEntryPoint); |
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.
If I understand correctly you need all the .csv files to inspect the graphs. In that case using this report
method is not ideal as it will timestamp each file (as @Sanne also noticed), potentially with a different timestamp. You could instead use com.oracle.graal.pointsto.reports.ReportUtils#report(java.lang.String, java.nio.file.Path, java.util.function.Consumer<java.io.PrintWriter>)
and provide a stable name for each file. To keep files organized you can write all .csv files for a run into a directory and timestamp the directory.
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.
I was trying to follow convention with the timestamp, but indeed as @Sanne noted, it makes scripting complicated. Timestamp the directory and you have the same issue. I think the report
folder is just fine as is, but we can use symlinks to link to the latest csv files, e.g. report/csv_call_tree_vm.csv
links to report/csv_call_tree_vm_123457890
. Whenever a new execution happens, the symlink is updated to the latest one. This would help with scripting providing stable paths.
substratevm/Reports.md
Outdated
@@ -147,3 +147,16 @@ The call tree report name has the structure `call_tree_<image_name>_<date_time>. | |||
The object tree report name has the structure: `object_tree_<image_name>_<date_time>.txt`. | |||
The image name is the name of the generated image, which can be set with the `-H:Name=<name>` option. | |||
The `<date_time>` is in the `yyyyMMdd_HHmmss` format. | |||
|
|||
#### CSV files |
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.
The Reports.md
has moved to ../docs/reference-manual/native-image/Reports.md
. Can you please rebase and add this documentation there?
@galderz please see my comments above. When those are solved I think this is ready to merge. |
95e5ae3
to
8896e6f
Compare
@cstancu I've rebased, the documentation should be in the right place. I've also gone ahead and implemented the symbolic link idea to provide stable paths for latest csv files. This should make it easier to script graph db vendor specific import steps. |
8896e6f
to
5f09a8b
Compare
There was a conflict with a recent commit. I've fixed that now. @cstancu can you have a look? |
Hmmm, hold on. I can't see the symbolic link changes any more... |
5f09a8b
to
db1420d
Compare
Fixed. I had fixed it in the wrong place. |
Any updates on this? @cstancu? |
* Methods and virtual methods are represented with graph nodes. * Direct, virtual and overriden-by relationships have been mapped. * Bytecode indexes are part of the relationships. * A method can interact with others multiple times, with each bytecode index indicating the origin of the call with the origin method.
db1420d
to
a641ae4
Compare
Your monthly reminder: any updates on this? |
Sorry for the tardiness in my reply, I will have it integrated shortly! |
Awesome, thanks all! |
Thx @cstancu!! |
This PR is a second version of the one proposed here.
It enhances the print analysis call tree to generate extra files that make it easy to import into graph databases such as Neo4j. It generates the following files:
To load the data into Neo4j, just call:
By loading the method universe into a graph database, issues such as infinispan/infinispan-quarkus#44 can be more easily debugged. The following query can help find the Infinispan source code path(s) that lead to the problematic location:
The Neo4j browser would present the output of this query in this way:
Other interesting queries for this use case are:
This PR improves on the previous PR:
display
field has been added that can be selected as field to show in the Neo4j browser. This field is a shortened version of thepackage+class name+method name
. The package is shortened with a letter per package, the class name is built out of the upper case letters (e.g. camel case) and the method name is included as is. This field helps share info with others more easily. With a live Neo4j instance, it is possible to click on each node and get more info.At this stage I'd like to hear feedback from other GraalVM contributors (@christianwimmer @olpaw ?) on the proposal above as well as the following points:
reports
folder...etc?