-
Notifications
You must be signed in to change notification settings - Fork 20
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
Added the JFR profiler to the profiler and rtsProfiler utils #91
Conversation
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.
Could you please write a few tests with the JFR option? Maybe do some assumeTrue
checks to ensure the minimum required Java version is present.
if (this.profilerChoice.equals("hprof")) { | ||
Map<String, Integer> methodCounts = Trace.fromHPROFFile(this.project, new UnitTest("", ""), profFile).methodCounts; | ||
|
||
hotMethods = methodCounts.entrySet() |
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 statement is repeated in the else block. It can be put outside the if
statement to avoid repetition.
profile_results.csv
Outdated
@@ -0,0 +1 @@ | |||
"Project","MethodIndex","Method","Count","Tests" |
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.
You can remove this file from the commit to avoid cluttering.
src/main/java/gin/util/Trace.java
Outdated
try (RecordingFile jfr = new RecordingFile(Paths.get(jfrF.getAbsolutePath()))) { | ||
|
||
//read all events from the JFR profiling file | ||
while (jfr.hasMoreEvents()) { |
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.
Indent
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 runs fine on Java 8 though I encountered trouble on Java 11; that appears to be unrelated to the profiling code though (seems to be with gradle sub projects). I've had a look over the changes and am happy with them. We might want to change the compiler flags (boolean might be better than the string options for saveChoice?).
@GiovaniGuizzo - have you any further comments before we accept?
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.
Sorry, I used my SBFT account. This is Giovani.
It is looking neat. Just change the string checks please.
@@ -62,10 +66,18 @@ public class Profiler implements Serializable { | |||
@Argument(alias = "hi", description="Interval for hprof's CPU sampling in milliseconds") | |||
protected Long hprofInterval = 10L; | |||
|
|||
@Argument(alias = "prof",description= "Profiler to use: JFR or HPROF. Default is JFR") |
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 documentation says either "JFR" or "HPROF". However, the checks are all lower case. I suggest when doing checks for this, use "toUpperCase().equals(anotherString)" to avoid problems.
src/main/java/gin/util/Profiler.java
Outdated
@@ -90,7 +102,9 @@ public Profiler(String[] args) { | |||
project.setMavenHome(this.mavenHome); | |||
} | |||
// Adds the interval provided by the user | |||
HPROF_ARG = HPROF_ARG.replace("$hprofInterval", Long.toString(hprofInterval)); | |||
if (this.profilerChoice.equals("hprof")) { |
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.
Example of lower case check.
src/main/java/gin/util/Profiler.java
Outdated
String args = HPROF_ARG + hprofFile(test, rep).getAbsolutePath(); | ||
String args; | ||
|
||
if (this.profilerChoice.equals("hprof")) { |
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.
Example of lower case check.
@Argument(alias = "prof", description = "Java hprof file name. If running in parallel, use a different name for each job.") | ||
private String profFileName = "java.prof.txt"; | ||
|
||
@Argument(alias = "prof",description= "Profiler to use: jfr or hprof. Default is jfr") |
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.
Same issue here. The documentation says "jfr" and "hprof" lower case and, although the checks are done in lower case, the user can set in upper case.
I think that's us all done. Thanks Myles for a very helpful contribution! |
No description provided.