jvm-profiling-tools / honest-profiler

A sampling JVM profiler without the safepoint sample bias
https://github.com/RichardWarburton/honest-profiler/wiki
MIT License
1.25k stars 146 forks source link

#185: Add method signatures to agent output #227

Closed ikavalio closed 6 years ago

ikavalio commented 6 years ago

This PR adds class signature, method descriptor and method signature to the log. There are not so many changes in agent code since all method calls are already there. I updated Java parser, so it will be able to understand new log entries. Method params and return type (non-generic) will be shown in console applications. com.insightfullogic.honest_profiler.core.parser.Method will store class and method signatures but ignore them for now, since pretty printing is a bit more tricky for signatures.

There's a tiny concurrent map change as well (add a copy constructor to map::Migration::Source), that allows agent to be compiled by gcc-4.4.

RichardWarburton commented 6 years ago

Thanks for the PR @ikavalio - generally looks good. I've just got one question on the PR about C_ATOMICS. Perhaps I'm just being dumb here, but would be good to understand what happens ;)

ikavalio commented 6 years ago

Hi Richard, thanks for reviewing this. It works fine without copy constructor on modern gcc's because they allow to use atomic instead of cstdatomic and it looks like a limitation of the latter. I wrote an example of code similar to map::Migration::Source that fails to compile on g++-4.4 unless copy constructor is defined. However it works perfectly on g++ > 4.6 no matter if copy constructor is defined or not. https://gist.github.com/ikavalio/0f4f81a0505d3b8a2adc91d31378ba4c.

I think it won't make any harm to define a copy constructor unconditionally instead, moreover it isn't used by the code.

RichardWarburton commented 6 years ago

Thanks for clarifying. If it isn't used in the code is there any harm in deleting it?

ikavalio commented 6 years ago

Implicit copy constructor will be created for map::Migration::Source even if we aren't using it in the code because it's used as a type param for std::vector. The problem is that copy constructor generated by compiler (if we don't provide it explicitly) isn't suitable/correct on g++-4.4 because of cstdatomic's std::atomic_int, but it is correct for standard atomic's std::atomic_int. So if I remove a copy constructor declaration it won't work on g++-4.4 and possibly some other versions. If I set it to delete (i.e. Source(const Source& s) = delete;) it won't work at all. But if we leave it as is or if I remove #ifdef/#enif and copy constructor is defined explicitly all the time it should work just fine for g++-4.4 and recent versions.

ikavalio commented 6 years ago

I completely agree that current ifdef is not very readable and clear way to do things, so I'll remove it and simply add a copy constructor to Source unconditionally.

RichardWarburton commented 6 years ago

Thanks @ikavalio - that makes a lot more sense now!