search

jvm-profiling-tools / perf-map-agent

A java agent to generate method mappings to use with the linux `perf` tool
GNU General Public License v2.0
1.65k stars 260 forks source link

Show Source Location in Map #20

Closed m4dc4p closed 8 years ago

m4dc4p commented 8 years ago

I've been profiling a lot of Scala code with this tool lately (so awesome, thank you!). The Scala compiler creates a lot of synthetic classes and methods, which can make it really hard to figure out where time is being spent when looking at a perf report. For example, you can get names like "com.foo.App$$anonfun$4$$anon$1.bar".

This PR adds an option, "showsource", which causes the agent to attach the name of the source file and the line on which it was defined to each JVM method in the perf map. To continue the example, the option would change the "name" of the method above to something like "com.foo.App$$anonfun$4$$anon$1.bar:(App.scala:20)".

I also cleaned up the shell scripts a bit. In particular, I added an environment variable, PERF_FLAME_OPTS, for passing options to flamegraph.pl.

jrudolph commented 8 years ago

Thanks @m4dc4p. I agree this is a useful addition. I would merge this PR and add another commit that would clean sig_string a bit: what do you think of it:

https://github.com/m4dc4p/perf-map-agent/compare/m4dc4p/show-source...jrudolph:w/simplify-sig_string?expand=1

m4dc4p commented 8 years ago

That looks good to me. I especially like how you simplified the creation of the final string and removed all those conditionals.

However, you need to set the JVM allocated buffers (csig, msig, etc.) to NULL when an error occurs, so the function doesn't accidentally call Deallocate on a bad pointer. According to the docs (http://docs.oracle.com/javase/7/docs/platform/jvmti/jvmti.html#functionReturn):

In the event that the JVM TI function encounters an error (any return value other than JVMTI_ERROR_NONE) the values of memory referenced by argument pointers is undefined, but no memory will have been allocated and no global references will have been allocated.

jrudolph commented 8 years ago

@m4dc4p good point. I will fix this and then merge.

jrudolph commented 8 years ago

@m4dc4p on second thought, I think nothing bad can happen as long as all Deallocate calls are guarded by a check of the return value of the get function. I fixed this in one remaining instance by adding another nesting of if clauses. Also, I found another bug in my previous code where I had overwritten msig leading to segfaults.

Thanks again!

jrudolph commented 8 years ago

Btw. I renamed the option to sourcepos.