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

The meaning of the "samples" count #365

Closed
Jongy opened this issue Mar 21, 2021 · 1 comment · Fixed by #366
Closed

The meaning of the "samples" count #365

Jongy opened this issue Mar 21, 2021 · 1 comment · Fixed by #366

Comments

@Jongy
Copy link
Contributor

Jongy commented Mar 21, 2021

Let's say I'm running py-spy record -o output -f raw --gil -d 3 -r 10 ... on an idle process. When done, py-spy will print happily:

py-spy> Wrote raw flamegraph data to 'output'. Samples: 30 Errors: 0

But reading the output file, it's actually empty. So why did it tell me it has 30 samples?

And on the other hand - should I run a multithreaded app, this time passing --idle, I'll get a graph with >30 samples.

In the "flamegraphs"/"stackcollapses" jargon, a sample is a single stack from a single thread. Linux's perf also uses "samples" this way. py-spy uses "samples" for "iterations of the profiling loop" - which is a number simply decided by duration * rate.

I think we should consider changing it, to be on-par with the naming used in other profilers. This "samples" count is misleading - it's not what you see after the profiling is done.

I thought it'd be a simple fix as:

diff --git a/src/main.rs b/src/main.rs
index 0435eb1..3358e97 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -251,7 +251,6 @@ fn record_samples(pid: remoteprocess::Pid, config: &Config) -> Result<(), Error>
             break;
         }
 
-        samples += 1;
         if let Some(max_samples) = max_samples {
             if samples >= max_samples {
                 exit_message = "";
@@ -287,6 +286,7 @@ fn record_samples(pid: remoteprocess::Pid, config: &Config) -> Result<(), Error>
             }
 
             output.increment(&trace)?;
+            samples += 1;
         }
 
         if let Some(sampling_errors) = sample.sampling_errors {

but that doesn't work out-of-the-box because we use the samples count to stop when reaching max_samples. So it'd require a few more changes I guess, which I'll happily do and open a PR, just tell me if you like this change @benfred .

@Jongy
Copy link
Contributor Author

Jongy commented Mar 21, 2021

diff --git a/src/main.rs b/src/main.rs
index 0435eb1..e4a5e38 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -215,6 +215,7 @@ fn record_samples(pid: remoteprocess::Pid, config: &Config) -> Result<(), Error>
     };
 
     let mut errors = 0;
+    let mut sampling_count = 0;
     let mut samples = 0;
     println!();
 
@@ -251,9 +252,9 @@ fn record_samples(pid: remoteprocess::Pid, config: &Config) -> Result<(), Error>
             break;
         }
 
-        samples += 1;
+        sampling_count += 1;
         if let Some(max_samples) = max_samples {
-            if samples >= max_samples {
+            if sampling_count >= max_samples {
                 exit_message = "";
                 break;
             }
@@ -268,6 +269,8 @@ fn record_samples(pid: remoteprocess::Pid, config: &Config) -> Result<(), Error>
                 continue;
             }
 
+            samples += 1;
+
             if config.include_thread_ids {
                 let threadid = trace.format_threadid();
                 trace.frames.push(Frame{name: format!("thread ({})", threadid),

seems to be enough, perhaps my naming could be improved haha (sampling_count??)

Jongy added a commit to Jongy/py-spy that referenced this issue Mar 21, 2021
This number is more useful for the user (as opposed to "the number of sampling intervals",
which we had until now). It matches the meaning of "samples" in other profiling tools,
e.g flamegraphs, Linux's "perf", and others.

We do not account samples whose recording is skipped: that is, GIL-less samples if --gil is given,
idle samples unless --idle is given.

Closes: benfred#365
benfred pushed a commit that referenced this issue Mar 22, 2021
This number is more useful for the user (as opposed to "the number of sampling intervals",
which we had until now). It matches the meaning of "samples" in other profiling tools,
e.g flamegraphs, Linux's "perf", and others.

We do not account samples whose recording is skipped: that is, GIL-less samples if --gil is given,
idle samples unless --idle is given.

Closes: #365
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 a pull request may close this issue.

1 participant