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

CPU flamegraphs using perf show wrong sample counts #326

Open
jg-linetco opened this issue Jan 5, 2024 · 1 comment
Open

CPU flamegraphs using perf show wrong sample counts #326

jg-linetco opened this issue Jan 5, 2024 · 1 comment

Comments

@jg-linetco
Copy link

jg-linetco commented Jan 5, 2024

Issue

When using the instruction from https://www.brendangregg.com/FlameGraphs/cpuflamegraphs.html the number of samples according to perf and the number of samples according to the flame graph differ greatly.
Example numbers:

perf record -e cpu-clock -ag -F 199 --call-graph dwarf -- sleep 10
...
[ perf record: Captured and wrote 1.090 MB perf.data (3980 samples) ]

The flame graph reports billions of samples:
image

Notes

The example flamegraph on https://www.brendangregg.com/FlameGraphs/cpuflamegraphs.html does report correct numbers.

First Analysis

This change has been introduced with #165 in PR #250

From there on, samples are weighted by a factor but still reported as "samples", making the output of perf and flamegraph contradictory.

@jg-linetco
Copy link
Author

IMHO we should bring back the old behavior and move the "weighting" behavior behind a command line option.

  1. The scripts should not silently add a weight to the samples and still report them as "samples". This makes the output confusing.
  2. The output of the script now does not match the documentation on the web site.
  3. The weighting is done using the number of cycles elapsed the last sample. I do not think this makes sense in all use cases, especially when dealing with samples that happen irregularly.
  4. The change hides the fact that we are still dealing with a bunch of samples and that we don't know what happens between the samples. Instead it tries to account for cycles, which only makes sense when the time between samples approaches 0.
  5. Other PRs are already trying to get the old behavior back: Add --counts option to ignore periods and just count stacks #297
  6. With older versions, some simple statistic checks were easy:
    "Is it only one sample? Then it has no significance, let's ignore it". Now we can't discern heavily weighted outliers from multiple, lower weighted samples that carry significant information.

I understand that there's demand to have the samples weighted by the amount the cycle counter increased between the last sample and this sample.

So my proposal would is:
Let's bring back the old behavior (sample count) as default behavior and add new command line option to enable the period-weighting of samples.

--periods "Weight samples according to the reported event period"

What do you think? I'll be happy to create the PR.
@kgibm already did most of the work in #297 ;-)

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

No branches or pull requests

1 participant