Perfexionists / perun

Lightweight Performance Control System
GNU General Public License v3.0
16 stars 14 forks source link

Add flamegraph diffs to the showdiff report #264

Closed JiriPavela closed 3 days ago

JiriPavela commented 4 days ago

Originally, the showdiff report had some code to generate flamegraph diff, but the call to the generate_flamegraphs function had skip_diff=True, so no diff would be generated.

This PR re-enables flamegraph generation and moreover:

JiriPavela commented 3 days ago

Can you elaborate in more details, what and why is changed here? It's not that clear, since flamegraph diffs were there initially as well.

Sure, originally, the showdiff report had some code to generate flamegraph diff, but the call to the generate_flamegraphs function had skip_diff=True, so no diff would be generated.

This PR re-enables flamegraph generation and moreover:

tfiedor commented 3 days ago

Can you elaborate in more details, what and why is changed here? It's not that clear, since flamegraph diffs were there initially as well.

Sure, originally, the showdiff report had some code to generate flamegraph diff, but the call to the generate_flamegraphs function had skip_diff=True, so no diff would be generated.

This PR re-enables flamegraph generation and moreover:

  • Both baseline-target and target-baseline diffs are generated separately and shown directly below baseline or target flamegraph, respectively. The motivation behind generating both comparisons is discussed here (Section "Negation").
  • All four flamegraphs are now unified: they have the same generation parameters where it makes sense (except minor differences, e.g., the --negation flag), they all respond to global search, have the same height and width, etc. Moreover, clicking into the diff flamegraph now opens the sankey graph as well.
  • The height resizing of SVG has been reworked. Previously, the height of the images was set according to the longest known traces, however, flamegraph.pl does some postprocessing and removes subtraces that don't meet certain width threshold. This caused some of the generated SVGs to have too much unnecessary blank space above the graphs. E.g., in one case, the longest trace was 34 while the postprocessed one was only 16, which caused the image to have more than double the necessary height and made it harder to compare all four graphs at the same time.

Nice, thank, sounds good. Please copy this to the PR description. Very good for fixing the height, it was quite hacked.