search

kornilova203 / FlameViewer

Tool for flamegraphs visualization
MIT License
80 stars 7 forks source link

Working on Issue #48. Instead of parsing the Java Flight Recordings a… #49

Closed brtubb-patagonia closed 5 years ago

brtubb-patagonia commented 5 years ago

…s a flamegraph file, which can be very large, we'll store as a compressed flamegraph file (flatbuffer: CFlamegraph). This currently does not work - com.github.kornilova_l.flight_parser.FlightParser, which currently handles the *.jfr parsing, does not support CFlamegraph.

--

Hi @kornilova-l, I made a bit more progress this attempt. We'll need add support for *.jfr parsing to CFlamegraph, the flatbuffer definition. The naive approach would be converting the current 'stacks' approach (FlightParser.getStacksMap => Map<String, Integer>) to CFlamegraph, but that'll be somewhat slow parsing Strings and converting. However, that may be better than modifying your FlightParser class to support CFlamegraph, as that'd couple the projects together.

This currently does not work, but properly calls the new conversion method JMCToCFlamegraphConverter.convert'. This new class is largely a copy of JMCConvert, but I didn't want to modify that due to the deprecated flag on the class.

brtubb-patagonia commented 5 years ago

Made a rough stab at implementing CFlamegraph parsing via the HashMap<String, Integer> we get from the FlightParser. Definitely not working when parsing the ClassName, MethodName, or Description when populating the lines, but it works some of the time. Hope that gets the intent across.

FYI, I think I found a possible bug. In multiple places in the code, HashMaps are used to manage the String/Integer mapping of class/method/description names. These get dumped to String arrays. Thing is, this doesn't preserve order. I think this may be resulting in the flamegraph's order of calls not being preserved. I'll need to test it more.

brtubb-patagonia commented 5 years ago

Oh, and sorry for lack of index out of bounds checks and other important checks. I do not consider this pull-ready; purely for WIP review.

kornilova203 commented 5 years ago

About stacks that are stored in HashMap: originally flamegraph loses stacks order because stacks of different time intervals are merged together if they are equal. TreesUtil.updateNodeList appends node such that nodes are in alphabetical order.

kornilova203 commented 5 years ago

If you face unknown problem try open build/idea-sandbox/system/log/idea.log (file in project directory) there are may be some exceptions

brtubb-patagonia commented 5 years ago

Thanks for the input @kornilova-l ! I'll review and make the changes. I won't be looking at this until Monday - the US has a national holiday tomorrow and we're off Friday as well.

Have a wonderful week and weekend!

brtubb-patagonia commented 5 years ago

Sure, will close and submit a new PR.