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

debugging: slides: properly introduce flamegraphs and hotspot #269

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Tropicao
Copy link
Contributor

@Tropicao Tropicao commented Mar 3, 2025

Flamegraphs, while being a very valuable tool to get a graphical view of some profiling data, are only lightly covered, very early, and somehow in the middle of other concepts.

Rework flamegraphs introduction to really introduce how to generate and read them:

  • move the flamegraph content in the system-wide profiling part, after having introduced the general perf commands, needed to generate flamegraph data
  • explain how to fetch the scripts and how to use them
  • add instructions about how to read a flamegraph, and a custom CPU flamegraph to illustrate it
  • add hotspot as an alternative/extension

The only downside is that, now that flamegraphs are introduced later, we reach the heaptrack part without having introduced flamegraphs, but it may be fine to just announce that "we will cover flamegraphs more in details later"

Fix #267

Preview of the new slides

Flamegraphs, while being a very valuable tool to get a graphical view of
some profiling data, are only lightly covered, very early, and somehow
in the middle of other concepts.

Rework flamegraphs introduction to really introduce how to generate and
read them:
- move the flamegraph content in the system-wide profiling part, after
  having introduced the general perf commands, needed to generate
  flamegraph data
- explain how to fetch the scripts and how to use them
- add instructions about how to read a flamegraph, and a custom CPU
  flamegraph to illustrate it
- add hotspot as an alternative/extension

The only downside is that, now that flamegraphs are introduced later, we
reach the heaptrack part without having introduced flamegraphs, but it
may be fine to just announce that "we will cover flamegraphs more in
details later"

Fix #267

Signed-off-by: Alexis Lothoré <[email protected]>
@Tropicao Tropicao requested a review from lucaceresoli March 3, 2025 21:22
Copy link
Contributor

@lucaceresoli lucaceresoli left a comment

Choose a reason for hiding this comment

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

Overall good, I just added a few small comments. Only one of them is very relevant with respect to the information provided by the slides.

\end{block}
\begin{itemize}
\item \code{stackcollapse-perf.pl} is part of the
\href{https://github.com/brendangregg/FlameGraph}{Flamegraph
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this bullet is needed, it's implied by the git clone line above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, agree. When I added this line, the git clone instruction was not here yet, so it kind of anticipated some questions, but now it does bring much. I'll remove it

\item Format the data:
\begin{block}{}
\begin{minted}[fontsize=\small]{console}
perf script|./fl/stackcollapse-perf.pl > out.perf-folded
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nit: I'd add a whitespace before and after the | for readability.

\center
\includegraphics[width=1.0\textwidth]{slides/debugging-system-wide-profiling/flamegraph.png}\\
\begin{itemize}
\item The plates width represents how often a function has been
Copy link
Contributor

Choose a reason for hiding this comment

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

"how often it has been sampled" or "for how long it has been active on the stack?" -- in other words, it represents number of events or execution time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically, it really is the first one. But then, assuming the sampling frequency is high enough, it gives an idea/estimate of the second one, relatively to anything else running on the platform.
One point I could add to clarify this is that when a plate is inside a "stack of plates", it has not specifically been sampled by perf/the kernel, it was just in the call stack of another function (let's call it foo) which as been actually sampled. And foo is then at the top of the "plate stack". Does it make more sense this way ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Modifications done and pushed in dedicated commits for review, those will be squashed before integration

@Tropicao Tropicao requested a review from lucaceresoli March 4, 2025 14:33
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.

Improve flamegraphs introduction and content
2 participants