search

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 145 forks source link

LogCollector Incorrectly Aggregates Top-level Frames #179

Open PhRX opened 7 years ago

PhRX commented 7 years ago

The LogCollector works based on the assumption that for any given thread, there's only 1 top-level method. That however isn't strictly true, looking at some real logs. The top of a stack typically is something like java.lang.Thread.run(), but in the main thread, the call can be preceded in time by classloading operations. For instance : sun.launcher.LauncherHelper.checkAndLoadMain(), at the same level as the Thread.run() call.

Because the aggregation assumes only 1 top-level frame, later stackframes may all end up in the wrong ProfileTree. In the example above, say that main calles com.a.b(), you might end up with an aggregated situation which looks like the sun.launcher.[...] is calling com.a.b().

This is fixed in my latest commit (the aggregation has been reworked), but that's on a dev branch which isn't yet integrated into the integration branch on my fork (and even further away from a Pull request).

nitsanw commented 7 years ago

Can you isolate the fix into a PR, or point to the fixing commit on your fork?

PhRX commented 7 years ago

@nitsanw Actually, that's not quite possible. I've reworked the aggregation, which is how I discovered it (by comparing the differences between what I'm seeing in my current build with the "original" HP UI). So the "fix" is inside the new classes, and can't really be isolated. Hopefully I can finish my current branch today (I need to add the filtering logic in the new structure), and integrate into the integration branch on my fork. The head there will then contain something which is functionally equivalent with what is at the head of the current Pull Request, but working on a new aggregation paradigm which is a bit more generic, and with the log collection decoupled more from the UI.

PhRX commented 7 years ago

Thinking of closing this but I didn't check the console. The issue is in LogCollector, so I suspect this issue should remain open until console is fixed too.