Add subinterpreter support#279
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #279 +/- ##
==========================================
- Coverage 79.05% 77.93% -1.12%
==========================================
Files 51 56 +5
Lines 5577 5988 +411
Branches 587 631 +44
==========================================
+ Hits 4409 4667 +258
- Misses 1168 1321 +153
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
scopreon
force-pushed
the
add_subinterpreter_support
branch
3 times, most recently
from
6e47658 to
c46223d
Compare
pablogsal
force-pushed
the
add_subinterpreter_support
branch
from
55139f0 to
77a8d89
Compare
|
After chatting with @godlygeek when he was here in London I have devised a way that we can use to make Note: this still needs refinement Example: |
This seems to be true, but it also doesn't seem to actually matter. If I understand correctly, you're talking about this assignment failing when there's multiple interpreters: if (tid_offset_in_pthread_struct == 0) {
tid_offset_in_pthread_struct = findPthreadTidOffset(manager, addr);
}and logging out LOG(ERROR) << "Could not find tid offset in pthread structure";when it does. But, if (manager->versionIsAtLeast(3, 11)) {
the_tid = ts.getField(&py_thread_v::o_native_thread_id);
} else {
the_tid = inferTidFromPThreadStructure(manager, pthread_id);
}I'm pretty sure that getting support for multiple interpreters working on Python 3.10 and earlier is out of scope for us, so I think we should just skip this call entirely when diff --git a/src/pystack/_pystack/pythread.cpp b/src/pystack/_pystack/pythread.cpp
index ae496e9..f287fb4 100644
--- a/src/pystack/_pystack/pythread.cpp
+++ b/src/pystack/_pystack/pythread.cpp
@@ -418,7 +409,7 @@ getThreadFromInterpreterState(
const std::shared_ptr<const AbstractProcessManager>& manager,
remote_addr_t addr)
{
- if (tid_offset_in_pthread_struct == 0) {
+ if (tid_offset_in_pthread_struct == 0 && !manager->versionIsAtLeast(3, 11)) {
tid_offset_in_pthread_struct = findPthreadTidOffset(manager, addr);
}That gets rid of the warning without needing any of the changes to Let me know if you disagree with that assessment, @scopreon! |
|
Thanks for the help taking a deep dive into this @pablogsal and @godlygeek , I haven't had a chance to take a proper look at the |
|
I've addressed all the comments I could although there are still a few unanswered qs, thanks for the feedback. Nice insight on the |
scopreon
force-pushed
the
add_subinterpreter_support
branch
from
baf7f42 to
c3aba54
Compare
|
Hi @pablogsal @godlygeek , thank you for your super useful help. I finally managed to find the time to resolve the more serious conflicts created from #272 (an impressive but scary PR). I've approximately ported the old code from 77a8d89 to use |
godlygeek
force-pushed
the
add_subinterpreter_support
branch
2 times, most recently
from
5260e6b to
827bdcc
Compare
Signed-off-by: Saul Cooperman <saulcoops@gmail.com>
Signed-off-by: Saul Cooperman <saulcoops@gmail.com>
Signed-off-by: Saul Cooperman <saulcoops@gmail.com>
Signed-off-by: Saul Cooperman <saulcoops@gmail.com>
Replace the `TracebackPrinter` class with a `print_threads()` function that detects shared-TID thread groups positionally. When threads from multiple subinterpreters share an OS thread, `format_thread()` now accepts `continuing_from_previous`/`continues_to_next` flags to emit a single "Traceback for thread" header with per-interpreter annotations like "In the main interpreter" or "In interpreter N". This relies on all of the interpreters' stacks for a given TID being printed sequentially. Previously that wasn't guaranteed, now it is. I've reworked `_normalize_threads()` to group threads by TID and sort each group by stack anchor even when native traces aren't enabled. This is what users would expect, anyway. Move the `frame_type()` classification back to Python, keeping only `is_eval_frame()` in C++ since that's what all native stack slicing needs. Remove the `py_isv314` version template. It's not necessary now that we only support using the `_Py_DebugOffsets` for 3.14+ and have dropped the hardcoded 3.14 offsets. Extract `num_entry_frames()` and `sort_threads_by_stack_anchor()` as standalone helpers. Fix the `std::accumulate` call to use a `size_t` rather than `int` for the accumulator. For Python < 3.7, `getInterpreterId()` now returns the interpreter address as a unique interpreter identifier instead of hardcoding 0. Signed-off-by: Matt Wozniski <mwozniski@bloomberg.net>
godlygeek
force-pushed
the
add_subinterpreter_support
branch
from
5094828 to
16d2ba6
Compare
Signed-off-by: Matt Wozniski <mwozniski@bloomberg.net>
This function is tricky enough to justify unit testing it, so expose it directly through pybind so we can exercise it with pytest. Signed-off-by: Matt Wozniski <mwozniski@bloomberg.net>
godlygeek
force-pushed
the
add_subinterpreter_support
branch
from
16d2ba6 to
f9e500d
Compare
|
I've spent some time iterating on this and I I've gotten it to a place where I'm almost ready to land it. I'm debating whether we should disable walking the interpreters list entirely for Python 3.12 and older. I was hoping we'd be able to support at least 3.12, but python/cpython#109860 makes it so that we can't reliably figure out which native thread a Python thread is running on. And any version older than 3.12 is probably a complete non-starter: the stack anchor approach that we're using to figure out what order the stacks for a given thread should go in relies on some having the FRAME_OWNED_BY_CSTACK shim frames that were added in 3.12. @scopreon if you get a chance, give my commits a review and see if there's anything you disagree with, please. |
Previously it was being duplicated in each of the `PyThreadData` in the vector, even though it's not a per-thread attribute. Signed-off-by: Matt Wozniski <mwozniski@bloomberg.net>
It's no longer necessary, we can now get a compile_commands.json
directly from CMake using:
pip install -v -e . --no-build-isolation \
-Cbuild-dir='build/{wheel_tag}' \
-Ccmake.define.CMAKE_EXPORT_COMPILE_COMMANDS=ON
Signed-off-by: Matt Wozniski <mwozniski@bloomberg.net>
While working on this PR I found myself repeatedly reimplementing and then removing this debug logging. Let's just leave it in, but make it conditional on `-vv` being used so that we don't pay the cost of formatting the debug info when no one is going to see it. Signed-off-by: Matt Wozniski <mwozniski@bloomberg.net>
Because all threads in a group share the same TID, they necessarily also share the same native thread, so the only two possibilities are that either all threads in the group have no native stack, or all threads in the group have a native stack. We only need to look at the first thread in the group to tell which of those two it is. Signed-off-by: Matt Wozniski <mwozniski@bloomberg.net>
|
OK, after my changes, the output now looks like this for one thread with multiple interpreters: Or this for a more involved example like the The output isn't changed for any thread with only one interpreter being used on it. Threads stacks from multiple interpreters have "In the main interpreter" or "In interpreter N" lines inserted at a half indent level to show where control leaves one interpreter and enters another, and those lines have the |
|
Thanks @godlygeek for tidying everything up, looks solid. The traceback looks neat with the GIL information etc. It's a shame that the 3.12 backport for python/cpython#109860 never made it in.
If we do allow walking the interpreter list, we should explicitly warn for versions <= 3.12 maybe? Or allow the ability to disable it. The only thing worse than a broken tool is one that breaks secretly/randomly (unless the failure mode is obvious?). Tbh I'm ok with disabling it entirely for <= 3.12 and just provide a reason in docs. Totally up to you. Other than that, no concerns on my wide wrt impl etc :) |
Yeah, I think that sounds best for now.
I think this mostly affects the
I've realized one other change that I should make here: right now I'm only showing which interpreter a thread is using when there's more than one interpreter on that thread, but I really need to show that whenever there's multiple interpreters in the process. Even if a given thread is specific to a single interpreter, someone reading the traceback would really want to know which of the N interpreters it's specific to, I think. |
godlygeek
force-pushed
the
add_subinterpreter_support
branch
from
a2a6cb7 to
ed98766
Compare
If there are multiple interpreters in use, always show which interpreter every thread is associated with. Signed-off-by: Matt Wozniski <mwozniski@bloomberg.net>
Since neither `concurrent.interpreters` nor `_interpreters` exists for Python 3.12, use the private `_xxsubinterpreters` module instead. Note that this requires us to change the test structure a bit to create an interpreter in the thread we first want to use it in, to avoid being bitten by gh-109860. Signed-off-by: Matt Wozniski <mwozniski@bloomberg.net>
This was the first version that introduced entry frames, and since it had entry frames but didn't yet have shim frames, we can use the address of the entry frame on the C stack as a stack anchor. Signed-off-by: Matt Wozniski <mwozniski@bloomberg.net>
Signed-off-by: Matt Wozniski <mwozniski@bloomberg.net>
godlygeek
force-pushed
the
add_subinterpreter_support
branch
from
ff82788 to
6e956e4
Compare
|
Alright, I've taken this a bit farther. I managed to get the tests working for 3.12 using I managed to also get things working for 3.11, which was the first version to add And I updated the behavior for 3.10 and earlier to simply log a warning if multiple interpreters are detected for Python 3.10 or earlier, and to ignore every interpreter but the last in the list (which should be the main interpreter, unless the main interpreter is dead but others are still alive somehow). I think that's the best we can do for those older versions, for which we have no stack anchor and so cannot determine the correct order to list the stacks for the multiple interpreters on a single OS thread in. I also made it so that, if we detected more than 1 interpreter in the process, we always log the interpreter in the traceback, even for threads with only one interpreter in use, or threads not associated with any interpreter. I assume this is what users are most likely to want to see while trying to make sense of multi-interpreter tracebacks. |
This test was passing everywhere but Alpine because GNU `env` takes a `-S` argument, but the busybox `env` in Alpine doesn't. We need to suppress the `-S` flag when we're running `env` instead of running a Python interpreter. Signed-off-by: Matt Wozniski <mwozniski@bloomberg.net>
Rather than sleeping and hoping we waited long enough for every worker thread and subinterpreter to start, use a pipe for synchronization within the test scripts just as we do between the pytest runner process and the test scripts. Signed-off-by: Matt Wozniski <mwozniski@bloomberg.net>
godlygeek
force-pushed
the
add_subinterpreter_support
branch
2 times, most recently
from
1065aa6 to
dd50a47
Compare
This will make it easier to maintain these compared to the previous nested triple quoted strings. Signed-off-by: Matt Wozniski <mwozniski@bloomberg.net>
godlygeek
force-pushed
the
add_subinterpreter_support
branch
from
dd50a47 to
584c66e
Compare
Signed-off-by: Matt Wozniski <mwozniski@bloomberg.net>
godlygeek
force-pushed
the
add_subinterpreter_support
branch
from
d7c3bb2 to
ff85c99
Compare
|
OK, I've cleaned up a few more small things here:
At this point, I think I'm ready to land this. @scopreon please give my latest changes a look if you can, and make sure I didn't screw anything up while polishing this. |
scopreon
left a comment
scopreon
left a comment
There was a problem hiding this comment.
LGTM. Have some comments, all nits so ffti
Signed-off-by: Matt Wozniski <mwozniski@bloomberg.net>
godlygeek
left a comment
godlygeek
left a comment
There was a problem hiding this comment.
OK, let's call this done. Thanks for bearing with us on this one, @scopreon - this turned out to be a much, much more involved change than I had originally anticipated. good first issue, I thought. Hah!
Thank you for participating in our trial by fire 😆
I'm very happy with where we wound up on this, though. I originally didn't think we'd be able to get hybrid stacks working, and getting that working makes this both cooler and more useful than I had originally hoped, so, yay!
4 participants
Issue number of the reported bug or feature request: #59,#280
Describe your changes
This PR adds simple
subinterpretersupport topystack. It preserves all original functionality but does it a few more times by traversing theinterpreterlinked list.Testing performed
Tests added
Additional context