Skip to content

Commit df0dc65

Browse files
authored
perf(profiling): consolidate greenlet switch updates (#19655)
## Description Consolidates the two native frame updates performed for each greenlet switch into one private `record_greenlet_switch()` call and one `greenlet_info_map` mutex acquisition. The existing behavior is preserved: the origin frame is always updated, while the running target frame is left unchanged when it is needed for parent-greenlet stack unwinding. This is an independently mergeable prerequisite extracted from #18724. ## Testing - `scripts/lint fmt -- ddtrace/profiling/_gevent.py tests/profiling/test_gevent.py tests/profiling/collector/test_stack.py` - `scripts/lint cformat` - `scripts/lint profiling-native-check` - `scripts/lint checks` - `git diff --check` - Fresh CPython 3.12 native extension build - Native linked-parent switch regression covering preserved and updated target frames - Existing high-cardinality greenlet switch contention regression ### Benchmark The benchmark isolates the native operation changed by this PR. Each measured run performs 1,000,000 logical greenlet switch updates after 10,000 warm-up updates: - baseline `e11d640110`: two `update_greenlet_frame()` native calls and two mutex acquisitions per update - candidate `93702898f8`: one `record_greenlet_switch()` native call and one mutex acquisition per update Both `_stack` extensions were built in Release mode from the exact commits, then run in the same container pinned to CPU 0 with `--cpuset-cpus=0`. Results over five runs: - before: 0.531681 seconds median, runs `[0.528339, 0.531681, 0.532128, 0.534566, 0.529234]` - after: 0.366206 seconds median, runs `[0.380415, 0.368066, 0.360803, 0.362898, 0.366206]` - change: approximately 31% faster for the isolated native update Environment: - AWS KVM VM, Intel Xeon Platinum 8175M at 2.50 GHz - 16 vCPUs, 8 cores with 2 threads per core, 61 GiB RAM - Linux 6.8.0-1055-aws, x86-64 - Docker server 29.5.2, container pinned to one CPU - CPython 3.12.13, GCC 14.2.0, glibc 2.41 - container image `sha256:ce7c46dbb7f07d352aecd756e19fd7f39550a72d4a13e68d30f02f5448eebd69` <details> <summary>Benchmark script</summary> ```python import statistics import time from ddtrace.internal.datadog.profiling import stack ITERATIONS = 1_000_000 RUNS = 5 ORIGIN_ID = 101 TARGET_ID = 102 stack.track_greenlet(ORIGIN_ID, "origin", False) stack.track_greenlet(TARGET_ID, "target", False) if hasattr(stack, "record_greenlet_switch"): def update(): stack.record_greenlet_switch(ORIGIN_ID, False, TARGET_ID, None, True) else: def update(): stack.update_greenlet_frame(ORIGIN_ID, False) stack.update_greenlet_frame(TARGET_ID, None) def run_once(): start = time.perf_counter() for _ in range(ITERATIONS): update() return time.perf_counter() - start for _ in range(10_000): update() runs = [run_once() for _ in range(RUNS)] stack.untrack_greenlet(ORIGIN_ID) stack.untrack_greenlet(TARGET_ID) print(f"median={statistics.median(runs):.6f}s runs={runs}") ``` </details> This is an isolated native-call benchmark, not an end-to-end application throughput claim. The earlier 67% figure was discarded after reproducing with exact baseline and candidate builds because the candidate run had not activated the profiling hook. ## Risks Low. The changed Python and native APIs are private and ship together. Existing parent-greenlet frame retention remains unchanged. ## Additional Notes The GIL release around native state mutation is retained intentionally. It was introduced by #14852 in commit `54a3a0ea35` to prevent potential thread-pool deadlocks while waiting for profiler mutexes. This change reduces two such release windows per greenlet switch to one. No release note is needed because this is an internal performance optimization with no user-facing API or behavior change. Co-authored-by: taegyun.kim <taegyun.kim@datadoghq.com>
1 parent 76b5d40 commit df0dc65

9 files changed

Lines changed: 107 additions & 28 deletions

File tree

ddtrace/internal/datadog/profiling/stack/__init__.pyi

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,13 @@ def init_asyncio(
9898
def track_greenlet(greenlet_id: int, name: str, frame: Union[FrameType, bool, None]) -> None: ...
9999
def untrack_greenlet(greenlet_id: int) -> None: ...
100100
def link_greenlets(greenlet_id: int, parent_id: int) -> None: ...
101-
def update_greenlet_frame(greenlet_id: int, frame: Union[FrameType, bool, None]) -> None: ...
101+
def record_greenlet_switch(
102+
origin_id: int,
103+
origin_frame: Union[FrameType, bool, None],
104+
target_id: int,
105+
target_frame: Union[FrameType, bool, None],
106+
update_target_frame: bool,
107+
) -> None: ...
102108

103109
# Module attributes
104110
is_available: bool

ddtrace/internal/datadog/profiling/stack/_stack.pyi

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,13 @@ def init_asyncio(
6666
def track_greenlet(greenlet_id: int, name: str, frame: Union[FrameType, bool, None]) -> None: ...
6767
def untrack_greenlet(greenlet_id: int) -> None: ...
6868
def link_greenlets(greenlet_id: int, parent_id: int) -> None: ...
69-
def update_greenlet_frame(greenlet_id: int, frame: Union[FrameType, bool, None]) -> None: ...
69+
def record_greenlet_switch(
70+
origin_id: int,
71+
origin_frame: Union[FrameType, bool, None],
72+
target_id: int,
73+
target_frame: Union[FrameType, bool, None],
74+
update_target_frame: bool,
75+
) -> None: ...
7076

7177
# Native call monitoring (sys.monitoring bridge)
7278
def start_native_monitoring() -> None: ...

ddtrace/internal/datadog/profiling/stack/include/sampler.hpp

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -136,7 +136,11 @@ class Sampler
136136
void track_greenlet(uintptr_t greenlet_id, TaskName name, PyObject* frame);
137137
void untrack_greenlet(uintptr_t greenlet_id);
138138
void link_greenlets(uintptr_t parent, uintptr_t child);
139-
void update_greenlet_frame(uintptr_t greenlet_id, PyObject* frame);
139+
void record_greenlet_switch(uintptr_t origin_id,
140+
PyObject* origin_frame,
141+
uintptr_t target_id,
142+
PyObject* target_frame,
143+
bool update_target_frame);
140144
void set_uvloop_mode(uintptr_t thread_id, bool value);
141145

142146
// The Python side dynamically adjusts the sampling rate based on overhead, so we need to be able to update our

ddtrace/internal/datadog/profiling/stack/src/echion/threads.cc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -604,7 +604,7 @@ ThreadInfo::unwind_greenlets(EchionSampler& echion, PyThreadState* tstate, unsig
604604

605605
// Phase 1: Snapshot greenlet data under the lock.
606606
// This minimises the time we hold greenlet_info_map_lock, which is also
607-
// acquired by update_greenlet_frame() on every greenlet switch. Holding
607+
// acquired by record_greenlet_switch() on every greenlet switch. Holding
608608
// the lock during the expensive unwind (Phase 2) would block ALL greenlet
609609
// switches and lead to resource exhaustion (e.g. DB connection pools).
610610
{

ddtrace/internal/datadog/profiling/stack/src/sampler.cpp

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -924,15 +924,22 @@ Sampler::link_greenlets(uintptr_t parent, uintptr_t child)
924924
}
925925

926926
void
927-
Sampler::update_greenlet_frame(uintptr_t greenlet_id, PyObject* frame)
927+
Sampler::record_greenlet_switch(uintptr_t origin_id,
928+
PyObject* origin_frame,
929+
uintptr_t target_id,
930+
PyObject* target_frame,
931+
bool update_target_frame)
928932
{
929933
std::lock_guard<std::mutex> guard(echion->greenlet_info_map_lock());
930-
931934
auto& greenlet_info_map = echion->greenlet_info_map();
932-
auto entry = greenlet_info_map.find(greenlet_id);
933-
if (entry != greenlet_info_map.end()) {
934-
// Update the frame of the greenlet
935-
entry->second->frame = frame;
935+
936+
if (auto origin = greenlet_info_map.find(origin_id); origin != greenlet_info_map.end()) {
937+
origin->second->frame = origin_frame;
938+
}
939+
if (update_target_frame) {
940+
if (auto target = greenlet_info_map.find(target_id); target != greenlet_info_map.end()) {
941+
target->second->frame = target_frame;
942+
}
936943
}
937944
}
938945

ddtrace/internal/datadog/profiling/stack/src/stack.cpp

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -510,16 +510,20 @@ link_greenlets(PyObject* Py_UNUSED(m), PyObject* args)
510510
}
511511

512512
static PyObject*
513-
update_greenlet_frame(PyObject* Py_UNUSED(m), PyObject* args)
513+
record_greenlet_switch(PyObject* Py_UNUSED(m), PyObject* args)
514514
{
515-
uintptr_t greenlet_id;
516-
PyObject* frame;
515+
uintptr_t origin_id;
516+
PyObject* origin_frame;
517+
uintptr_t target_id;
518+
PyObject* target_frame;
519+
int update_target_frame;
517520

518-
if (!PyArg_ParseTuple(args, "lO", &greenlet_id, &frame))
521+
if (!PyArg_ParseTuple(args, "lOlOp", &origin_id, &origin_frame, &target_id, &target_frame, &update_target_frame))
519522
return nullptr;
520523

521524
Py_BEGIN_ALLOW_THREADS;
522-
Sampler::get().update_greenlet_frame(greenlet_id, frame);
525+
Sampler::get().record_greenlet_switch(
526+
origin_id, origin_frame, target_id, target_frame, static_cast<bool>(update_target_frame));
523527
Py_END_ALLOW_THREADS;
524528

525529
Py_RETURN_NONE;
@@ -1023,7 +1027,7 @@ static PyMethodDef stack_methods[] = {
10231027
{ "track_greenlet", track_greenlet, METH_VARARGS, "Map a greenlet with its identifier" },
10241028
{ "untrack_greenlet", untrack_greenlet, METH_VARARGS, "Untrack a terminated greenlet" },
10251029
{ "link_greenlets", link_greenlets, METH_VARARGS, "Link two greenlets" },
1026-
{ "update_greenlet_frame", update_greenlet_frame, METH_VARARGS, "Update the frame of a greenlet" },
1030+
{ "record_greenlet_switch", record_greenlet_switch, METH_VARARGS, "Record a greenlet context switch" },
10271031

10281032
{ "set_adaptive_sampling", stack_set_adaptive_sampling, METH_VARARGS, "Set adaptive sampling" },
10291033
{ "set_target_overhead",

ddtrace/internal/datadog/profiling/stack/test/test_sampling_cycle_state.cpp

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
#include "echion/echion_sampler.h"
22
#include "echion/task_name.h"
3+
#include "sampler.hpp"
34

45
#include <gtest/gtest.h>
56

@@ -36,3 +37,44 @@ TEST(SamplingCycleState, UnwindReplacesTaskAndGreenletStacksFromPriorCycle)
3637
EXPECT_TRUE(thread.current_tasks.empty());
3738
EXPECT_TRUE(thread.current_greenlets.empty());
3839
}
40+
41+
TEST(SamplingCycleState, GreenletSwitchPreservesLinkedParentFrame)
42+
{
43+
constexpr GreenletInfo::ID child_id = 101;
44+
constexpr GreenletInfo::ID parent_id = 102;
45+
PyObject child_running_frame{};
46+
PyObject child_suspended_frame{};
47+
PyObject parent_suspended_frame{};
48+
PyObject parent_resumed_frame{};
49+
50+
Datadog::Sampler& sampler = Datadog::Sampler::get();
51+
EchionSampler& echion = sampler.get_echion();
52+
{
53+
std::lock_guard<std::mutex> guard(echion.greenlet_info_map_lock());
54+
auto& greenlets = echion.greenlet_info_map();
55+
greenlets.emplace(
56+
child_id, std::make_unique<GreenletInfo>(child_id, &child_running_frame, TaskName::from_literal("child")));
57+
greenlets.emplace(
58+
parent_id,
59+
std::make_unique<GreenletInfo>(parent_id, &parent_suspended_frame, TaskName::from_literal("parent")));
60+
}
61+
sampler.link_greenlets(parent_id, child_id);
62+
63+
sampler.record_greenlet_switch(child_id, &child_suspended_frame, parent_id, &parent_resumed_frame, false);
64+
{
65+
std::lock_guard<std::mutex> guard(echion.greenlet_info_map_lock());
66+
EXPECT_EQ(echion.greenlet_parent_map().at(child_id), parent_id);
67+
EXPECT_EQ(echion.greenlet_info_map().at(child_id)->frame, &child_suspended_frame);
68+
EXPECT_EQ(echion.greenlet_info_map().at(parent_id)->frame, &parent_suspended_frame);
69+
}
70+
71+
sampler.record_greenlet_switch(child_id, &child_running_frame, parent_id, &parent_resumed_frame, true);
72+
{
73+
std::lock_guard<std::mutex> guard(echion.greenlet_info_map_lock());
74+
EXPECT_EQ(echion.greenlet_info_map().at(child_id)->frame, &child_running_frame);
75+
EXPECT_EQ(echion.greenlet_info_map().at(parent_id)->frame, &parent_resumed_frame);
76+
echion.greenlet_info_map().erase(child_id);
77+
echion.greenlet_info_map().erase(parent_id);
78+
echion.greenlet_parent_map().erase(child_id);
79+
}
80+
}

ddtrace/profiling/_gevent.py

Lines changed: 21 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -66,9 +66,17 @@ def track_gevent_greenlet(gl: _Greenlet, _from_tracer: bool = False) -> _Greenle
6666
return gl
6767

6868

69-
def update_greenlet_frame(greenlet_id: int, frame: t.Union[FrameType, bool, None]) -> None:
70-
_tracked_greenlets.add(greenlet_id)
71-
stack.update_greenlet_frame(greenlet_id, frame)
69+
def record_greenlet_switch(
70+
origin_id: int,
71+
origin_frame: t.Union[FrameType, bool, None],
72+
target_id: int,
73+
target_frame: t.Union[FrameType, bool, None],
74+
update_target_frame: bool,
75+
) -> None:
76+
_tracked_greenlets.add(origin_id)
77+
if update_target_frame:
78+
_tracked_greenlets.add(target_id)
79+
stack.record_greenlet_switch(origin_id, origin_frame, target_id, target_frame, update_target_frame)
7280

7381

7482
def greenlet_tracer(event: str, args: t.Any) -> None:
@@ -97,16 +105,18 @@ def greenlet_tracer(event: str, args: t.Any) -> None:
97105
try:
98106
# If this is being set to None, it means the greenlet is likely
99107
# finished. We use the sentinel again to signal this.
100-
update_greenlet_frame(
108+
origin_frame = t.cast(t.Optional[FrameType], origin.gr_frame) or FRAME_NOT_SET
109+
# We don't want to wipe the frame of a parent greenlet because
110+
# we need to unwind it. We definitely know it is still running
111+
# so if we allow the tracer to set its tracked frame to None,
112+
# we won't be able to unwind the full stack.
113+
record_greenlet_switch(
101114
origin_id,
102-
t.cast(t.Optional[FrameType], origin.gr_frame) or FRAME_NOT_SET,
115+
origin_frame,
116+
target_id,
117+
target.gr_frame, # This is None for the running target.
118+
target_id not in _parent_greenlet_count,
103119
)
104-
if target_id not in _parent_greenlet_count:
105-
# We don't want to wipe the frame of a parent greenlet because
106-
# we need to unwind it. We definitely know it is still running
107-
# so if we allow the tracer to set its tracked frame to None,
108-
# we won't be able to unwind the full stack.
109-
update_greenlet_frame(target_id, target.gr_frame) # this *is* None
110120
except KeyError:
111121
# TODO: Log missing greenlet
112122
pass

tests/profiling/collector/test_stack.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1099,7 +1099,7 @@ def test_gevent_greenlet_switch_not_blocked_by_profiler() -> None:
10991099
11001100
Before the fix, unwind_greenlets() held greenlet_info_map_lock for the
11011101
entire stack unwinding of ALL tracked greenlets. Every greenlet switch
1102-
calls update_greenlet_frame() under the same lock, so more tracked
1102+
calls record_greenlet_switch() under the same lock, so more tracked
11031103
greenlets meant longer lock hold and more switch blocking.
11041104
11051105
This test measures greenlet-switch wall time with zero vs many idle

0 commit comments

Comments
 (0)