GeezerVison

All commit authors signed the Contributor License Agreement.

Will try to review this week. Heads up that we are very close to beta freeze so we would need to land this this week or next week at the latest

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

CC @vstinner @lysnikolaou

It was proposed to make PyUnstable_DumpTraceback() public: see PR gh-148145. IMO it would already be a first step forward. I prefer to wait until that's done to consider adding another similar-but-different API.

Thanks for the review @pablogsal and @vstinner. I talked with @pablogsal offline, and I think we can indeed push most of the work onto the caller. The one remaining piece is a signal-safe way to walk the stack frames themselves, which I think can be done with a fairly small API change. I'll test this out and get back to you with an updated diff :)

Documentation build overview

📚 cpython-previews | 🛠️ Build #33188738 | 📁 Comparing 912299e against main (448d7b9)

  🔍 Preview build  

@pablogsal I have reworked the proposal to add the missing iterator function, with the rest of the work able to happen on the client side. LMK if this looks any better :)

It's hard or even impossible to write a reliable Python in-process debugger/profiler. For example, HEAD_LOCK() is missing whereas it's needed to iterate safely on interpreters and thread states: see issue gh-85234. Interpreters, thread states, frames and code objects can be added and destroyed anytime. For example, it's possible to get crashes when running faulthandler on a program which creates/destroys many threads in parallel while faulthandler is running: #116008 (comment).

"Safe" in _PyFrame_SafeGetCode() name is a trap. It's not safe, it can still crash if the frame became a dangling pointer in the meanwhile, and it returns a borrowed reference whereas the code object can be destroyed anytime.

faulthandler has to use heuristics such as checking _PyObject_IsFreed() to reduce the risk of crashes. But it's only a weak workaround, not a reliable solution. faulthandler uses a signal handler and so cannot use the Python C API or even allocate memory on the heap.

An out-of-process debugger/profiler is more reliable since it gets an error instead of a stack fault when reading invalid memory locations.

Building a more reliable in-process debugger/profiler would require synchronization and so call functions, change reference count, allocate memory on the heap, etc. It would be incompatible with a signal hander.

Thanks for the links @vstinner.

There are use-cases I can see for an API like this, and I believe 2 of them still apply.

1. The first use-case is profiling from an in-process hook/wrapper (e.g. allocation/live-heap/lock/io profiling). In these cases, the system is in a stable state and the GIL is held (if in non-freethreaded mode). This is how tracemalloc works as I understand it, and we would like to build similar observability in userspace. The problem is that the current public API for stackwalking potentially reenters the interpreter, which makes reasoning about correctness difficult. Are there issues that would prevent a non-reentrant API working in this case?
2. The second use-case is crash-reporting: i.e. doing what the current fault-handler does, but in userspace to generate structured data instead of free-text. In this case, any operations are clearly only best effort since we're inside a crashing program. I believe this case is still valuable, but with better naming to indicate that while the functions do not signal-unsafe operations like retentering the interpreter or allocating memory, this does not mean they are “safe”, merely “less unsafe”.
3. The third is an in-process signal-based CPU profiler, and I agree that sadly this wouldn’t work for that. I will try to propose better naming to indicate that this is not a supported use-case at this time.

@vstinner @pablogsal I have updated the function names and descriptions to follow the discussion above.

Adding a new C API for in-process debugger/profiler would mean that we endorse this use case. I would prefer to not endorse it for the reason Pablo and me gave above. So I don't think that's a good idea to add such C API.

For example, PyUnstable_ThreadState_GetCurrentFrame() doesn't hold a strong reference and so can become a dangling pointer anytime. Other functions have a similar issue.

I understand some of the raised concerns have been discussed offline, so apologies if any of the following comments are redundant or moot at this point.

The majority of production Python profilers are out-of-process.

I feel that some of the (valid) points raised in previous comments hinge on putting all tools such as profilers into the same basket. In my experience, there is a set of categories that one would have to consider. Not all profilers are the same, and some make sense as out-of-process, but others are only viable as in-process. A sampling profiler (for wall/CPU time) is something that one might want to implement as an out-of-process tool, for various reasons. The stochastic nature of the tool means that a few incorrect stack walks can be tolerated. However, memory profilers cannot rely on a similar mechanism if they are to run with performance decent enough to be used in production. That's because the inevitable upscaling from a sampling mechanism would combine with potentially incorrect stacks, leading to an unacceptable compounding of errors. Therefore, I would argue that memory profilers would require efficient, in-process stack-walking logic.

It's hard or even impossible to write a reliable Python in-process debugger/profiler.

My understanding is that the proposed changes are not addressing the general (and hard) problem of adding an API for any kind of profilers/debuggers. The main scope of the proposed API is to provide efficient utilities that can be used for memory profiling. When we specialise the problem to just this case, I believe there are many simplifying assumptions that can be made. For once, grabbing a stacktrace from a hook triggered by a memory allocation will run effectively in-line with the allocation. This pins the call stack of the thread the unwinder is running on, meaning that one can assume that internal structures like frames, thread states, and everything else related to the thread are not going to mutate under the unwinder's feet. Surely some degree of caution is still required, but the problem is much more tractable than the general case.

But landing it here flips the direction of the maintenance burden

I agree that the burden of maintaining the new API to serve tools should not fall on CPython in general. However, my view is that, if we can keep the changes to a minimum and the new API surface as lean as possible, the trade-off would be that of potentially giving CPython an efficient built-in memory profiler that could well complement the newly introduced sampling profiler.

An out-of-process debugger/profiler is more reliable since it gets an error instead of a stack fault when reading invalid memory locations.

This is probably off-topic, but there is nothing preventing an in-process tool from using the same out-of-process techniques to read memory safely. Of course the shared resources in this case imply e.g. a different performance profile, but the point is that there are techniques that can be used to make in-process tools almost as safe as out-of-process ones.

This PR is stale because it has been open for 90 days with no activity.