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

[GR-34638] Introduce PrintAnalysisCallTreeType option #3861

Closed
wants to merge 3 commits into from

Conversation

zakkak
Copy link
Collaborator

@zakkak zakkak commented Oct 5, 2021

This option enables users to choose the structure of the call tree report. Currently supported formats are txt and csv.

The use of this flag implicitly enables PrintAnalysisCallTree option.

Closes: #3843

@zakkak
Copy link
Collaborator Author

zakkak commented Oct 5, 2021

@galderz please review

Copy link
Contributor

@galderz galderz left a comment

Choose a reason for hiding this comment

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

Minor changes.

@fniephaus fniephaus changed the title Introduce PrintAnalysisCallTreeType option [GR-34638 Introduce PrintAnalysisCallTreeType option Oct 20, 2021
@fniephaus fniephaus changed the title [GR-34638 Introduce PrintAnalysisCallTreeType option [GR-34638] Introduce PrintAnalysisCallTreeType option Oct 20, 2021
@fniephaus fniephaus self-assigned this Oct 20, 2021
@@ -52,6 +53,15 @@ protected void onValueUpdate(EconomicMap<OptionKey<?>, Object> values, Boolean o
}
};

@Option(help = "Change the output format of the analysis call tree. See: Reports.md.")//
Copy link
Member

Choose a reason for hiding this comment

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

Since you are referring to Reports.md it would be nice to mention this option in the #### Call tree section there.

Copy link
Member

Choose a reason for hiding this comment

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

From that help message the user would not know that the two possible option values are TXT and CSV.

toCsvFile("call tree for direct edges", reportsPath, "csv_call_tree_direct_edges", reportName, writer -> printBciEdges(directEdges, writer));
toCsvFile("call tree for overriden by edges", reportsPath, "csv_call_tree_override_by_edges", reportName, writer -> printNonBciEdges(overridenByEdges, writer));
toCsvFile("call tree for virtual edges", reportsPath, "csv_call_tree_virtual_edges", reportName, writer -> printBciEdges(virtualEdges, writer));
toCsvFile("call tree csv file for vm entry point", reportsPath, "call_tree_vm", reportName, CallTreePrinter::printVMEntryPoint);
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to use

Sting msgPrefix = "call tree csv file for "

with

...
         toCsvFile(msgPrefix + "entry point", reportsPath ....
...

This option enables users to choose the structure of the call tree
report. Currently supported formats are txt and csv.

The use of this flag implicitly enables PrintAnalysisCallTree option.

Closes: oracle#3843
@zakkak
Copy link
Collaborator Author

zakkak commented Nov 17, 2021

Thank you for the review @olpaw . All comments should now be resolved.

Copy link
Member

@olpaw olpaw left a comment

Choose a reason for hiding this comment

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

Looking good now. @fniephaus please take it from here.

@fniephaus
Copy link
Member

Looking good now. @fniephaus please take it from here.

Done... the PR is in internal review and should be merged soon.

@zakkak
Copy link
Collaborator Author

zakkak commented Nov 23, 2021

Closed via a134ee4

@zakkak zakkak closed this Nov 23, 2021
@zakkak zakkak deleted the fix-3843 branch November 23, 2021 13:36
@zakkak
Copy link
Collaborator Author

zakkak commented Nov 23, 2021

Now that this got merged on master would it be possible to backport to 21.3 as well? (cc @fniephaus)

@fniephaus
Copy link
Member

Why does this need to be backported to 21.3? It seems it was already backported in Mandrel?

@zakkak
Copy link
Collaborator Author

zakkak commented Dec 22, 2021

Why does this need to be backported to 21.3? It seems it was already backported in Mandrel?

As discussed in graalvm#302 we decided to avoid deviating from upstream GraalVM, since that could confuse users.

In general we prefer the upstream-first approach unless something is a blocker or brings a very important feature. In this case, it looks like this feature is more of a convenience-thing so we decided not to deviate from upstream GraalVM.

Note, however, that this is still something we would like to see landing in 21.3 at some point if possible.

@zakkak
Copy link
Collaborator Author

zakkak commented Mar 10, 2022

Hi @fniephaus are there any updates regarding getting this backported to 21.3?

@fniephaus
Copy link
Member

Apologies, this slipped under my radar. I've opened a backporting issue for this internally and will monitor its progress.

@fniephaus
Copy link
Member

I don't know when the 21.3 release branch will be updated but we've backported this internally so it will be part of the next 21.3 release.

@zakkak
Copy link
Collaborator Author

zakkak commented Mar 16, 2022

Thanks a lot @fniephaus !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[GR-34638] Add option to pick which kind of reports one wants to generate
5 participants