From 1e58b3ff0b0e046b84b86747b1c11c61ff44334c Mon Sep 17 00:00:00 2001 From: Nick Ripley Date: Fri, 17 Jan 2025 16:10:40 +0000 Subject: [PATCH] refactor(profiling): add more GIL assertions to memalloc The first GIL assertion I added, to memalloc_add_event, has not tripped yet on a test application. One the one hand, it's reassuring that we always see the GIL in that part of the code. On the other hand, there are other parts of the memory profiler that could in theory be called concurrently where I didn't add the assertion: when stopping the profiler, and when iterating over the events to aggregate them. Add GIL assertions to those points. The goal is ultimately to understand why we needed to add locks to the profiler to prevent it from crashing, given that the GIL exists. --- ddtrace/profiling/collector/_memalloc.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/ddtrace/profiling/collector/_memalloc.c b/ddtrace/profiling/collector/_memalloc.c index b55e9ebcfab..1f2b87e0433 100644 --- a/ddtrace/profiling/collector/_memalloc.c +++ b/ddtrace/profiling/collector/_memalloc.c @@ -109,13 +109,19 @@ memalloc_init() } static void -memalloc_add_event(memalloc_context_t* ctx, void* ptr, size_t size) +memalloc_assert_gil() { if (g_crash_on_no_gil && !PyGILState_Check()) { int* p = NULL; *p = 0; abort(); // should never reach here } +} + +static void +memalloc_add_event(memalloc_context_t* ctx, void* ptr, size_t size) +{ + memalloc_assert_gil(); uint64_t alloc_count = atomic_add_clamped(&global_alloc_tracker->alloc_count, 1, ALLOC_TRACKER_MAX_COUNT); @@ -332,6 +338,8 @@ memalloc_stop(PyObject* Py_UNUSED(module), PyObject* Py_UNUSED(args)) return NULL; } + memalloc_assert_gil(); + PyMem_SetAllocator(PYMEM_DOMAIN_OBJ, &global_memalloc_ctx.pymem_allocator_obj); memalloc_tb_deinit(); if (memlock_trylock(&g_memalloc_lock)) { @@ -389,6 +397,8 @@ iterevents_new(PyTypeObject* type, PyObject* Py_UNUSED(args), PyObject* Py_UNUSE if (!iestate) return NULL; + memalloc_assert_gil(); + /* reset the current traceback list */ if (memlock_trylock(&g_memalloc_lock)) { iestate->alloc_tracker = global_alloc_tracker;