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.

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

Flame Graph #502

Merged
merged 3 commits into from
Jan 8, 2018
Merged

Flame Graph #502

merged 3 commits into from
Jan 8, 2018

Conversation

adamsitnik
Copy link
Member

This PR is recreated #440

PerfView is very powerful tool, but sometimes for some inexperienced users like me it is hard to get a fast overview of what takes most of the CPU if there are plenty of methods on the stack.

This PR provides a simple implementation of the "Flame Graph".

It is just different visualization of the "call tree" view.

Sample result (for the "tutorial.exe")

image

Sample result for more complicated app (dotnet cli):

image

  1. I added new tab (before the "notes")
  2. I did not use SVG (most common way of implementing flame graph) because I would have to add external dependency to the project (link)
  3. I decided to draw the rectangles/textblocks on a simple WPF Canvas. For small boxes, I use rectangles (too many textblocks on a single canvas = bad perf)
  4. I have used some fancy color pallet to differentiate the boxes
  5. I added possibility to export the flame graph to png file (right click => save)
  6. I added the docs
  7. The canvas is redrawn on resize/when the tab gets the focus
  8. Both tool tip and bottom panel contains method name, samples count and % of total time
  9. Since the flame graph is using data from call tree view, any filter applied to call tree view works for flame graph as well

@vancem
Copy link
Contributor

vancem commented Jan 8, 2018

@adamsitnik There is a failure in the debug test run. I looked into it a bit and it seems likely that it is issue #354 causihg the issue.

You need to update StackWindows to that the TestIncludeItemOnCallerCalleeTabCallerAsync test has the folloiwng

        [WpfFact(Skip = "Failing with indexOutOfRange and Debug testing.   See issue https://github.com/Microsoft/perfview/issues/354")]

@vancem
Copy link
Contributor

vancem commented Jan 8, 2018

PerfView has a mechanism where all files needed at runtime are emedded into the EXE itself and extracted at first launch. You added a new *.PNG file which needs to be emedded into PerfView.exe.

To do this modify src\PerfView\PerfVIew.csproj to add the following lines

    <EmbeddedResource Include="SupportFiles\Images\FlameGraphView.png">
      <Type>Non-Resx</Type>
      <WithCulture>false</WithCulture>
      <LogicalName>.\Images\FlameGraphView.png</LogicalName>
    </EmbeddedResource>

at line 417 (after all the other 'EmbeddedResource elements), but within the ItemGroup element.

@sharwell
Copy link
Member

sharwell commented Jan 8, 2018

📝 I restarted the build, which may pass on second try with no changes.

Edit: Yes, it passed.

@@ -0,0 +1,160 @@
using System;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please put the FlameGraph.cs file in the 'src\perfView\StackViewer' directory.

It gives you very intelligible overview.&nbsp;&nbsp;
The graph starts at the bottom. Each box represents a method in the stack. Every parent is the caller, children are the callees.
The wider the box, the more time it was on-CPU. The samples count is shown in the tooltip and in the bottom panel.
To change the content of the flame graph you need to apply the filters for call tree view.&nbsp;&nbsp;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you put a hyperlink to some documentation on how to use FlameGraphs that already exists somewhere on the internet?

@codecov-io
Copy link

codecov-io commented Jan 8, 2018

Codecov Report

Merging #502 into master will decrease coverage by 0.12%.
The diff coverage is 6.71%.

@@            Coverage Diff             @@
##           master     #502      +/-   ##
==========================================
- Coverage   17.53%   17.41%   -0.13%     
==========================================
  Files         216      217       +1     
  Lines      123479   123627     +148     
  Branches    11910    11927      +17     
==========================================
- Hits        21658    21530     -128     
- Misses     100959   101231     +272     
- Partials      862      866       +4
Flag Coverage Δ
#2017 17.41% <6.71%> (-0.13%) ⬇️
#Debug 17.41% <6.71%> (-0.13%) ⬇️
Impacted Files Coverage Δ
src/PerfView.Tests/StackViewer/StackWindowTests.cs 76.36% <ø> (-18.21%) ⬇️
src/PerfView/StackViewer/FlameGraph.cs 0% <0%> (ø)
src/PerfView/StackViewer/StackWindow.xaml 100% <100%> (ø) ⬆️
src/PerfView/StackViewer/StackWindow.xaml.cs 25.25% <13.2%> (-0.93%) ⬇️
src/PerfView/GuiUtilities/GuiUtilities.cs 9.42% <0%> (-6.81%) ⬇️
src/PerfView/StackViewer/CallerCalleeView.xaml.cs 43.02% <0%> (-5.82%) ⬇️
src/TraceEvent/Stacks/CallTree.cs 46.52% <0%> (-3.39%) ⬇️
.../PerfView/GuiUtilities/StatusBar/StatusBar.xaml.cs 57.22% <0%> (-3.18%) ⬇️
src/PerfView/StackViewer/CallTreeView.cs 47.89% <0%> (-1.06%) ⬇️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 36de508...a4fdefc. Read the comment docs.

@vancem
Copy link
Contributor

vancem commented Jan 8, 2018

Please update the perfView version number (in the Directory.Build.Props file (it is in the 'Solution Items' folder in the solution). to be 2.0.1 and then udpate the 'ReleaseNotes' section of the UsersGuid.htm to indicate that flame graphs have been added in this version of PerfView).

@@ -411,6 +412,8 @@ public void SetStackSource(StackSource newSource, Action onComplete = null)
cumMax / 1000000, controller.GetStartTimeForBucket((HistogramCharacterIndex)cumMaxIdx));
}

RedrawFlameGraph();
Copy link
Contributor

Choose a reason for hiding this comment

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

I would like this to be lazy. Simply have a bit that marks that the flame graph is invalid at this point. Then have an event when the canvas becomes visible that actually calls the RedrawFlamGraph.

This avoids a bunch of work that may never be used (if users never look at the flamegraph.

Copy link
Contributor

@vancem vancem left a comment

Choose a reason for hiding this comment

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

I have left a number of small change requests (the biggest being to delay redrawing the flamegraph until the view is visable). Other than those I am OK with this change.

@adamsitnik
Copy link
Member Author

@vancem Done! Good idea with redrawing the graph only when it is visible.

@vancem vancem merged commit f1731b1 into microsoft:master Jan 8, 2018
@vancem
Copy link
Contributor

vancem commented Jan 8, 2018

Looks good to me

@goldshtn
Copy link

🎊 Very happy to see this merged! Nowadays I find that flame graphs are more useful than stack trees or other forms of stack visualization in 99% of the cases. Minor nitpick: if we could get rid of the "module" noise so that as much as possible of the actual function name would be shown in the flame rectangles, it would be even more awesome...

@adamsitnik
Copy link
Member Author

Minor nitpick: if we could get rid of the "module" noise so that as much as possible of the actual function name would be shown in the flame rectangles, it would be even more awesome...

ok.

Once you start using it and have some more comments please create a separate issue. I would like to make the Flame Graphs user-friendly. And I did not forget about the colors, I just did not figure this out yet.

@xiangzhai
Copy link

:mips-interest

@adamsitnik adamsitnik deleted the flameGraph branch July 10, 2020 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants