Skip to content

Commit ecac33a

Browse files
committed
gh-159312: Prefer bound stacks for CPU sample attribution
1 parent 82b612d commit ecac33a

6 files changed

Lines changed: 221 additions & 5 deletions

File tree

‎Lib/test/test_external_inspection.py‎

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2596,6 +2596,109 @@ def test_unsupported_platform_error(self):
25962596

25972597
@requires_remote_subprocess_debugging()
25982598
class TestDetectionOfThreadStatus(RemoteInspectionTestBase):
2599+
def _check_cpu_status_with_multiple_thread_states(
2600+
self, *, native=False, subinterpreter=False,
2601+
):
2602+
import_module("_testinternalcapi")
2603+
import_module("_interpreters")
2604+
if native:
2605+
import_module("_hashlib")
2606+
work = "_hashlib.pbkdf2_hmac('sha256', b'password', b'salt', 10_000_000)"
2607+
else:
2608+
work = "pass"
2609+
code = textwrap.dedent(f"""
2610+
import socket
2611+
{"import _hashlib" if native else ""}
2612+
active_sock = socket.socket(fileno={{fd}})
2613+
2614+
def active():
2615+
active_sock.sendall(b"ready")
2616+
while True:
2617+
{work}
2618+
2619+
active()
2620+
""")
2621+
script = f"""
2622+
import _interpreters, _testinternalcapi
2623+
2624+
def suspended():
2625+
if {subinterpreter}:
2626+
interpid = _interpreters.create()
2627+
else:
2628+
interpid = _interpreters.get_current()[0]
2629+
# Create another state on this OS thread.
2630+
code = {code!r}.format(fd=sock.fileno())
2631+
_testinternalcapi.exec_interpreter(interpid, code)
2632+
2633+
suspended()
2634+
"""
2635+
with self._target_process(script) as (p, sock, _):
2636+
_wait_for_signal(sock, b"ready")
2637+
for mode in (PROFILING_MODE_CPU, PROFILING_MODE_ALL):
2638+
with self.subTest(mode=mode):
2639+
unwinder = RemoteUnwinder(
2640+
p.pid, all_threads=True, mode=mode,
2641+
skip_non_matching_threads=(mode == PROFILING_MODE_CPU),
2642+
)
2643+
for _ in busy_retry(SHORT_TIMEOUT):
2644+
try:
2645+
trace = unwinder.get_stack_trace()
2646+
except TRANSIENT_ERRORS:
2647+
continue
2648+
active = self._find_thread_with_frame(
2649+
trace, lambda f: f.funcname == "active")
2650+
if active is None or not active.status & THREAD_STATUS_ON_CPU:
2651+
continue
2652+
if native and active.status & THREAD_STATUS_HAS_GIL:
2653+
continue
2654+
suspended = self._find_thread_with_frame(
2655+
trace, lambda f: f.funcname == "suspended")
2656+
if mode == PROFILING_MODE_CPU:
2657+
self.assertIsNone(suspended)
2658+
else:
2659+
self.assertIsNotNone(suspended)
2660+
self.assertEqual(suspended.thread_id, active.thread_id)
2661+
self.assertFalse(suspended.status & THREAD_STATUS_ON_CPU)
2662+
break
2663+
2664+
@skip_if_not_supported
2665+
def test_cpu_status_with_multiple_thread_states(self):
2666+
for native in (False, True):
2667+
for subinterpreter in (False, True):
2668+
with self.subTest(native=native, subinterpreter=subinterpreter):
2669+
self._check_cpu_status_with_multiple_thread_states(
2670+
native=native, subinterpreter=subinterpreter)
2671+
2672+
@skip_if_not_supported
2673+
def test_cpu_status_after_tstate_callback(self):
2674+
import_module("_testinternalcapi")
2675+
for retain in (False, True):
2676+
with self.subTest(retain=retain):
2677+
script = f"""
2678+
import _testinternalcapi
2679+
2680+
def ready():
2681+
sock.sendall(b"ready")
2682+
2683+
def native_work():
2684+
_testinternalcapi.native_work_after_tstate_callback(
2685+
ready, {SHORT_TIMEOUT * 3}, {retain})
2686+
2687+
native_work()
2688+
"""
2689+
with self._target_process(script) as (p, sock, _):
2690+
_wait_for_signal(sock, b"ready")
2691+
for mode in (PROFILING_MODE_CPU, PROFILING_MODE_ALL):
2692+
with self.subTest(mode=mode):
2693+
unwinder = RemoteUnwinder(p.pid, all_threads=True, mode=mode)
2694+
for _ in busy_retry(SHORT_TIMEOUT):
2695+
trace = _get_stack_trace_with_retry(unwinder)
2696+
thread = self._find_thread_with_frame(
2697+
trace, lambda f: f.funcname == "native_work")
2698+
if thread and thread.status & THREAD_STATUS_ON_CPU:
2699+
self.assertFalse(thread.status & THREAD_STATUS_HAS_GIL)
2700+
break
2701+
25992702
def _run_thread_status_test(self, mode, check_condition):
26002703
"""
26012704
Common pattern for thread status detection tests.
Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
Fix :mod:`profiling.sampling` attributing CPU samples to suspended thread states
2+
when multiple thread states share the same native thread.

‎Modules/_remote_debugging/_remote_debugging.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -689,7 +689,8 @@ extern PyObject* unwind_stack_for_thread(
689689
uintptr_t gil_holder_tstate,
690690
uintptr_t gc_frame,
691691
uintptr_t main_thread_tstate,
692-
const RemoteReadPrefetch *prefetch
692+
const RemoteReadPrefetch *prefetch,
693+
int *has_bound_stack
693694
);
694695

695696
/* Thread stopping functions (for blocking mode) */

‎Modules/_remote_debugging/module.c‎

Lines changed: 63 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -650,6 +650,45 @@ read_interp_state_and_maybe_thread_frame(
650650
interp_state_buffer);
651651
}
652652

653+
static int
654+
filter_cpu_thread_states(PyObject *result, PyObject *bound_threads, int skip)
655+
{
656+
// Prefer a bound stack when states share an OS thread. There may be no
657+
// bound state after a native callback deletes its temporary thread state.
658+
for (Py_ssize_t i = 0; i < PyList_GET_SIZE(result); i++) {
659+
PyObject *threads = PyStructSequence_GET_ITEM(PyList_GET_ITEM(result, i), 1);
660+
for (Py_ssize_t j = PyList_GET_SIZE(threads); j-- > 0;) {
661+
PyObject *thread = PyList_GET_ITEM(threads, j);
662+
PyObject *bound = PyDict_GetItemWithError(
663+
bound_threads, PyStructSequence_GET_ITEM(thread, 0));
664+
if (bound == NULL) {
665+
if (PyErr_Occurred()) {
666+
return -1;
667+
}
668+
continue;
669+
}
670+
long status = PyLong_AsLong(PyStructSequence_GET_ITEM(thread, 1));
671+
if (bound == thread || !(status & THREAD_STATUS_ON_CPU)) {
672+
continue;
673+
}
674+
if (skip) {
675+
if (PySequence_DelItem(threads, j) < 0) {
676+
return -1;
677+
}
678+
}
679+
else {
680+
PyObject *new_status = PyLong_FromLong(status & ~THREAD_STATUS_ON_CPU);
681+
if (new_status == NULL) {
682+
return -1;
683+
}
684+
Py_DECREF(PyStructSequence_GET_ITEM(thread, 1));
685+
PyStructSequence_SET_ITEM(thread, 1, new_status);
686+
}
687+
}
688+
}
689+
return 0;
690+
}
691+
653692
/*[clinic input]
654693
@critical_section
655694
_remote_debugging.RemoteUnwinder.get_stack_trace
@@ -714,6 +753,15 @@ _remote_debugging_RemoteUnwinder_get_stack_trace_impl(RemoteUnwinderObject *self
714753
set_exception_cause(self, PyExc_MemoryError, "Failed to create stack trace result list");
715754
return NULL;
716755
}
756+
PyObject *bound_threads = NULL;
757+
if (self->tstate_addr == 0 &&
758+
(self->mode == PROFILING_MODE_CPU || self->mode == PROFILING_MODE_ALL)) {
759+
bound_threads = PyDict_New();
760+
if (bound_threads == NULL) {
761+
Py_DECREF(result);
762+
return NULL;
763+
}
764+
}
717765

718766
// Iterate over all interpreters
719767
uintptr_t current_interpreter = self->interpreter_addr;
@@ -802,11 +850,13 @@ _remote_debugging_RemoteUnwinder_get_stack_trace_impl(RemoteUnwinderObject *self
802850

803851
while (current_tstate != 0) {
804852
uintptr_t prev_tstate = current_tstate;
853+
int has_bound_stack;
805854
PyObject* frame_info = unwind_stack_for_thread(self, &current_tstate,
806855
gil_holder_tstate,
807856
gc_frame,
808857
main_thread_tstate,
809-
&prefetch);
858+
&prefetch,
859+
&has_bound_stack);
810860
if (!frame_info) {
811861
// Check if this was an intentional skip due to mode-based filtering
812862
if ((self->mode == PROFILING_MODE_CPU || self->mode == PROFILING_MODE_GIL ||
@@ -832,10 +882,13 @@ _remote_debugging_RemoteUnwinder_get_stack_trace_impl(RemoteUnwinderObject *self
832882
goto exit;
833883
}
834884

835-
if (PyList_Append(interpreter_threads, frame_info) == -1) {
885+
if (PyList_Append(interpreter_threads, frame_info) == -1 ||
886+
(bound_threads && has_bound_stack &&
887+
PyDict_SetItem(bound_threads,
888+
PyStructSequence_GET_ITEM(frame_info, 0), frame_info) < 0)) {
836889
Py_DECREF(frame_info);
837890
Py_DECREF(interpreter_threads);
838-
set_exception_cause(self, PyExc_RuntimeError, "Failed to append thread frame info");
891+
set_exception_cause(self, PyExc_RuntimeError, "Failed to collect thread frame info");
839892
Py_CLEAR(result);
840893
goto exit;
841894
}
@@ -890,7 +943,14 @@ _remote_debugging_RemoteUnwinder_get_stack_trace_impl(RemoteUnwinderObject *self
890943
}
891944
}
892945

946+
if (bound_threads != NULL &&
947+
filter_cpu_thread_states(result, bound_threads,
948+
self->mode == PROFILING_MODE_CPU && self->skip_non_matching_threads) < 0) {
949+
Py_CLEAR(result);
950+
}
951+
893952
exit:
953+
Py_XDECREF(bound_threads);
894954
// Invalidate cache entries for threads not seen in this sample.
895955
// Only do this every 1024 iterations to avoid performance overhead.
896956
if (self->cache_frames && result) {

‎Modules/_remote_debugging/threads.c‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -335,7 +335,8 @@ unwind_stack_for_thread(
335335
uintptr_t gil_holder_tstate,
336336
uintptr_t gc_frame,
337337
uintptr_t main_thread_tstate,
338-
const RemoteReadPrefetch *prefetch
338+
const RemoteReadPrefetch *prefetch,
339+
int *has_bound_stack
339340
) {
340341
PyObject *frame_info = NULL;
341342
PyObject *thread_id = NULL;
@@ -618,6 +619,10 @@ unwind_stack_for_thread(
618619
goto error;
619620
}
620621

622+
*has_bound_stack = GET_MEMBER(
623+
_thread_status, ts, unwinder->debug_offsets.thread_state.status).bound_gilstate
624+
&& PyList_GET_SIZE(frame_info) != 0;
625+
621626
// py_status contains status flags (bitfield)
622627
PyStructSequence_SetItem(result, 0, thread_id);
623628
PyStructSequence_SetItem(result, 1, py_status); // Steals reference

‎Modules/_testinternalcapi.c‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2372,6 +2372,49 @@ destroy_interpreter(PyObject *self, PyObject *args, PyObject *kwargs)
23722372
Py_RETURN_NONE;
23732373
}
23742374

2375+
static PyObject *
2376+
native_work_after_tstate_callback(PyObject *self, PyObject *args)
2377+
{
2378+
PyObject *callback;
2379+
double seconds;
2380+
int retain;
2381+
if (!PyArg_ParseTuple(args, "Odp", &callback, &seconds, &retain)) {
2382+
return NULL;
2383+
}
2384+
PyTime_t start, now;
2385+
if (PyTime_Monotonic(&start) < 0) {
2386+
return NULL;
2387+
}
2388+
PyThreadState *temporary = PyThreadState_New(PyInterpreterState_Get());
2389+
if (temporary == NULL) {
2390+
return PyErr_NoMemory();
2391+
}
2392+
PyThreadState *saved = PyThreadState_Swap(temporary);
2393+
PyObject *result = PyObject_CallNoArgs(callback);
2394+
PyObject *exc = result ? NULL : PyErr_GetRaisedException();
2395+
if (retain) {
2396+
PyThreadState_Swap(NULL);
2397+
}
2398+
else {
2399+
PyThreadState_Clear(temporary);
2400+
PyThreadState_DeleteCurrent();
2401+
}
2402+
// Continue native work without reattaching the original thread state.
2403+
while (result != NULL && PyTime_MonotonicRaw(&now) == 0 &&
2404+
PyTime_AsSecondsDouble(now - start) < seconds) {
2405+
}
2406+
if (retain) {
2407+
PyThreadState_Swap(temporary);
2408+
PyThreadState_Clear(temporary);
2409+
PyThreadState_DeleteCurrent();
2410+
}
2411+
PyThreadState_Swap(saved);
2412+
if (exc != NULL) {
2413+
PyErr_SetRaisedException(exc);
2414+
}
2415+
return result;
2416+
}
2417+
23752418
// This exists mostly for testing the _interpreters module, as an
23762419
// alternative to _interpreters.destroy()
23772420
static PyObject *
@@ -3409,6 +3452,8 @@ static PyMethodDef module_functions[] = {
34093452
METH_VARARGS | METH_KEYWORDS},
34103453
{"destroy_interpreter", _PyCFunction_CAST(destroy_interpreter),
34113454
METH_VARARGS | METH_KEYWORDS},
3455+
{"native_work_after_tstate_callback", native_work_after_tstate_callback,
3456+
METH_VARARGS, NULL},
34123457
{"exec_interpreter", _PyCFunction_CAST(exec_interpreter),
34133458
METH_VARARGS | METH_KEYWORDS},
34143459
{"run_in_subinterp_with_config",

0 commit comments

Comments
 (0)