Skip to content

Commit

Permalink
refactor(profiling): add more GIL assertions to memalloc
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
nsrip-dd committed Jan 17, 2025
1 parent f67a358 commit 1e58b3f
Showing 1 changed file with 11 additions and 1 deletion.
12 changes: 11 additions & 1 deletion ddtrace/profiling/collector/_memalloc.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down Expand Up @@ -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)) {
Expand Down Expand Up @@ -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;
Expand Down

0 comments on commit 1e58b3f

Please sign in to comment.