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

Pause target process when retrieving stack frames #316

Merged
merged 4 commits into from Apr 19, 2021

Conversation

acj
Copy link
Member

@acj acj commented Apr 18, 2021

We don't currently pause the target process when calling get_stack_frame, which results in incorrect and misleading profiler output like we see in #315. The remoteprocess crate has support for pausing processes on each platform. The output seems much more accurate in my testing, so I'm going to enable it.

Using higher sampling frequencies will degrade performance of the target process while rbspy is running, but 1) this is already true, at least on some systems, because of higher CPU usage, and 2) it feels like a reasonable tradeoff for the sake of correctness. If we find that it's a problem, we could consider adding a flag to disable pausing.

Fixes #153. Fixes #315.

@jvns
Copy link
Collaborator

jvns commented Apr 18, 2021

It looks like this is py-spy's approach -- pause by default and have a --nonblocking flag to disable pausing https://github.com/benfred/py-spy#how-can-i-avoid-pausing-the-python-program.

@benfred
Copy link
Collaborator

benfred commented Apr 18, 2021

I did some testing on locking with rbspy when I originally put the locking code into py-spy - my takeaway was that locking the process isn't as important with rbspy as it is with py-spy, because of how stacks are represented internally on the python/ruby interpreters.

I'll write up a quick blog post on this in the next couple days going into some more details on this, but the big takeaway is that the errors from not locking are much worse with py-spy than they are with rbspy.

This locking code obviously helps in some situations (like #315) - so is good to add. However, I think we should add a commandline flag to turn off/on the locking code, and that the default for rbspy maybe should be to run without locking - like the opposite of py-spy which defaults to locking. (I think the fact that this hasn't been raised as an issue in several years for rbspy but was flagged with py-spy in its first 2 months speaks to the relative importance of locking with rbspy/py-spy).

@acj
Copy link
Member Author

acj commented Apr 18, 2021

Thanks for the context @benfred. I like the --nonblocking flag and am happy to add it before this is merged.

I'm pretty firmly on the side of doing the safe and correct thing by default -- it's what I expect from this type of tool. If someone wants to knowingly turn off a correctness feature to improve performance, that's fine, but IMO it should be opt-out.

It's a fair point that we haven't gotten many complaints related to this type of thing. I think the stack correctness issues may have been obscured by other bugs that have been fixed recently, and folks may have used rbspy to get a rough initial guess at where the slow spots are before switching to something more invasive (and, currently, more accurate) like stackprof. If they felt more confident in rbspy's output, it might make a difference in their chosen workflow. That was the case for me when I started working with it -- it was incredible but a bit rough around the edges, so I wanted a second tool's opinion -- and is definitely influencing my feelings here.

I'll write up a quick blog post on this in the next couple days

Looking forward to that 👍

@acj acj merged commit 48e2417 into rbspy:master Apr 19, 2021
@acj acj deleted the lock-process-each-frame branch April 19, 2021 20:51
@benfred
Copy link
Collaborator

benfred commented Nov 9, 2021

I finally got around to writing that post on the nonblocking mode for rbspy vs py-spy :
https://www.benfrederickson.com/why-python-needs-paused-during-profiling/

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.

Inconsistent stack samples Support pausing the process while getting the stack
3 participants