Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add flamegraph diffs to the showdiff report #264

Merged
merged 2 commits into from
Nov 3, 2024
Merged

Add flamegraph diffs to the showdiff report #264

merged 2 commits into from
Nov 3, 2024

Conversation

JiriPavela
Copy link
Collaborator

@JiriPavela JiriPavela commented Nov 3, 2024 •
edited
Loading

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.

@JiriPavela
Add flamegraph diffs to the showdiff report
ee799cc 
Both baseline-target and target-baseline diffs are generated
and displayed.

Also, the height computation of the generated images has been
adjusted to reduce the amount of unused space.
tfiedor
tfiedor approved these changes Nov 3, 2024
View reviewed changes
Copy link
Collaborator

@tfiedor tfiedor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

perun/view/flamegraph/flamegraph.py Outdated
title = title if title != "" else f"{profile_type} consumption of {cmd} {workload}"
units = mapping.get_unit(mapping.get_readable_key(profile_key))

# flame = convert.to_flame_graph_format(profile, profile_key=profile_key, minimize=minimize)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this commented out here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, this comment shouldn't be there. I'll remove it.

@JiriPavela
Copy link
Collaborator Author

JiriPavela commented Nov 3, 2024 •
edited
Loading

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.

@tfiedor
Copy link
Collaborator

tfiedor commented Nov 3, 2024

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.

@JiriPavela JiriPavela force-pushed the showdiff-report-diff-flamegraphs branch from b375796 to 768f8cd Compare November 3, 2024 19:39
@JiriPavela JiriPavela merged commit 8ed8f17 into Perfexionists:devel Nov 3, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tfiedor tfiedor tfiedor approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@JiriPavela @tfiedor