Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-128679: Redesign tracemalloc locking #128888

Merged
merged 5 commits into from
Jan 15, 2025
Merged

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Jan 15, 2025

  • Use TABLES_LOCK() to protect 'tracemalloc_config.tracing'.
  • Hold TABLES_LOCK() longer while accessing tables.
  • tracemalloc_realloc_gil() and tracemalloc_raw_realloc() no longer remove the trace on reentrant call.
  • _PyTraceMalloc_Stop() unregisters _PyTraceMalloc_TraceRef().
  • _PyTraceMalloc_GetTraces() sets the reentrant flag.
  • tracemalloc_clear_traces_unlocked() sets the reentrant flag.

* Use TABLES_LOCK() to protect 'tracemalloc_config.tracing'.
* Hold TABLES_LOCK() longer while accessing tables.
* tracemalloc_realloc_gil() and tracemalloc_raw_realloc() no longer
  remove the trace on reentrant call.
* _PyTraceMalloc_Stop() unregisters _PyTraceMalloc_TraceRef().
* _PyTraceMalloc_GetTraces() sets the reentrant flag.
* tracemalloc_clear_traces_unlocked() sets the reentrant flag.
@vstinner
Copy link
Member Author

_PyTraceMalloc_Stop() unregisters _PyTraceMalloc_TraceRef().

This change cannot be backported to Python 3.12 which doesn't have the PyRefTracer_SetTracer() function.

Anyway, I don't plan to backport this major redesign to Python 3.13 neither.

Don't remove the trace if it's a reentrant call.
Copy link
Member

@ZeroIntensity ZeroIntensity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a much better solution. A few notes:

  • I think it's worth trying to migrate away from PyThread, because it's a lot more difficult to work with than the modern APIs. We can go with PyMutex and _Py_thread_local instead.
  • We need to be careful with holding locks while acquiring the GIL, because that can lead to lock-ordering deadlocks.
  • I'll need to look into what kind of subinterpreter problems might come up because of PyGILState_Ensure, as it doesn't work very well. What would happen if e.g. a thread calls PyTraceMalloc_Track, which sets the GILstate for the main interpreter, then enters a different interpreter?

@vstinner vstinner enabled auto-merge (squash) January 15, 2025 20:16
@vstinner vstinner merged commit 36c5e3b into python:main Jan 15, 2025
39 checks passed
@vstinner vstinner deleted the tracemalloc_lock branch January 15, 2025 20:22
@vstinner
Copy link
Member Author

I think it's worth trying to migrate away from PyThread, because it's a lot more difficult to work with than the modern APIs. We can go with PyMutex and _Py_thread_local instead.

Why not. It can be done afterwards.

We need to be careful with holding locks while acquiring the GIL, because that can lead to lock-ordering deadlocks.

Right. The order is: first get the GIL, and then call TABLES_LOCK().

I'll need to look into what kind of subinterpreter problems might come up because of PyGILState_Ensure, as #123728. What would happen if e.g. a thread calls PyTraceMalloc_Track, which sets the GILstate for the main interpreter, then enters a different interpreter?

This PR doesn't change that. But right, there should be corner cases when sub-interpreters are involved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants