gh-115184: Fix refleak tracking issues in free-threaded build #115188
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- We did not account for abandoned segments, which could miss blocks in multithreaded runs. - The mimalloc heap traversal needed to call "_mi_page_free_collect" earlier in order to get an accurate count of blocks in use. - `_Py_DecRefSharedDebug` was missing a `_Py_IncRefTotal`, but this was mostly offset by a missing accounting in `_Py_ExplicitMergeRefcount`. - get_num_global_allocated_blocks should pause other threads to ensure that traversing the mimalloc heaps is safe.
colesbury
added
the
🔨 test-with-refleak-buildbots
|
🤖 New build scheduled with the buildbot fleet by @colesbury for commit 1b17640 🤖 If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again. |
bedevere-bot
removed
the
🔨 test-with-refleak-buildbots
DinoV
reviewed
Feb 9, 2024
| #ifdef Py_REF_DEBUG | ||
| _Py_AddRefTotal(_PyInterpreterState_GET(), extra); | ||
| #endif | ||
|
|
||
| _Py_atomic_store_uint32_relaxed(&op->ob_ref_local, 0); |
There was a problem hiding this comment.
Do I understand this correctly that we're not racing here because we're either own the owning thread or the world is stopped for GC?
There was a problem hiding this comment.
The possible cases are:
- We are the owning thread and gave up ownership (typical)
- We are the queueing thread and the owning thread has exited (gh-110481: Implement inter-thread queue for biased reference counting #114824)
- We are the queueing thread and have stopped the world (due to OOM) (gh-110481: Implement inter-thread queue for biased reference counting #114824)
The GC could have used this code path, but it uses a different function that avoids atomics (because it's stopped the world).
The reason case (2) is thread-safe is a bit subtle:
- Queueing is done once, so only one thread will attempt it
- Although thread ids may be reused, the merge for (2) happens while holding a lock that would need to be acquired by any new thread that reuses the thread id.
fsc-eriker
pushed a commit
to fsc-eriker/cpython
that referenced
this pull request
Feb 14, 2024
…ython#115188) Fixes a few issues related to refleak tracking in the free-threaded build: - Count blocks in abandoned segments - Call `_mi_page_free_collect` earlier during heap traversal in order to get an accurate count of blocks in use. - Add missing refcount tracking in `_Py_DecRefSharedDebug` and `_Py_ExplicitMergeRefcount`. - Pause threads in `get_num_global_allocated_blocks` to ensure that traversing the mimalloc heaps is safe.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
_mi_page_free_collectearlier in order to get an accurate count of live blocks._Py_DecRefSharedDebugwas missing a_Py_IncRefTotal, but this was mostly offset by a missing accounting in_Py_ExplicitMergeRefcount.