Skip to content

gh-115184: Fix refleak tracking issues in free-threaded build#115188

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.

Sign up for GitHub

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

Merged
colesbury merged 1 commit into from
Feb 9, 2024
+16 −6
Merged

gh-115184: Fix refleak tracking issues in free-threaded build #115188

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Objects/mimalloc/heap.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -538,7 +538,6 @@ bool _mi_heap_area_visit_blocks(const mi_heap_area_t* area, mi_page_t *page, mi_
mi_assert(page != NULL);
if (page == NULL) return true;

_mi_page_free_collect(page,true);
mi_assert_internal(page->local_free == NULL);
if (page->used == 0) return true;

Expand DownExpand Up@@ -635,6 +634,7 @@ bool _mi_heap_area_visit_blocks(const mi_heap_area_t* area, mi_page_t *page, mi_
typedef bool (mi_heap_area_visit_fun)(const mi_heap_t* heap, const mi_heap_area_ex_t* area, void* arg);

void _mi_heap_area_init(mi_heap_area_t* area, mi_page_t* page) {
_mi_page_free_collect(page,true);
const size_t bsize = mi_page_block_size(page);
const size_t ubsize = mi_page_usable_block_size(page);
area->reserved = page->reserved * bsize;
Expand Down
11 changes: 7 additions & 4 deletions Objects/object.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -346,6 +346,9 @@ _Py_DecRefSharedDebug(PyObject *o, const char *filename, int lineno)
if (should_queue) {
// TODO: the inter-thread queue is not yet implemented. For now,
// we just merge the refcount here.
#ifdef Py_REF_DEBUG
_Py_IncRefTotal(_PyInterpreterState_GET());
#endif
Py_ssize_t refcount = _Py_ExplicitMergeRefcount(o, -1);
if (refcount == 0) {
_Py_Dealloc(o);
Expand DownExpand Up@@ -399,17 +402,17 @@ _Py_ExplicitMergeRefcount(PyObject *op, Py_ssize_t extra)
Py_ssize_t shared = _Py_atomic_load_ssize_relaxed(&op->ob_ref_shared);
do {
refcnt = Py_ARITHMETIC_RIGHT_SHIFT(Py_ssize_t, shared, _Py_REF_SHARED_SHIFT);
if (_Py_REF_IS_MERGED(shared)) {
return refcnt;
}

refcnt += (Py_ssize_t)op->ob_ref_local;
refcnt += extra;

new_shared = _Py_REF_SHARED(refcnt, _Py_REF_MERGED);
} while (!_Py_atomic_compare_exchange_ssize(&op->ob_ref_shared,
&shared, new_shared));

#ifdef Py_REF_DEBUG
_Py_AddRefTotal(_PyInterpreterState_GET(), extra);
#endif

_Py_atomic_store_uint32_relaxed(&op->ob_ref_local, 0);

@DinoVDinoVFeb 9, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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?

@colesburycolesburyFeb 9, 2024

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

The possible cases are:

  1. We are the owning thread and gave up ownership (typical)
  2. We are the queueing thread and the owning thread has exited (gh-110481: Implement inter-thread queue for biased reference counting #114824)
  3. 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.

_Py_atomic_store_uintptr_relaxed(&op->ob_tid, 0);
return refcnt;
Expand Down
9 changes: 8 additions & 1 deletion Objects/obmalloc.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -1073,7 +1073,12 @@ get_mimalloc_allocated_blocks(PyInterpreterState *interp)
mi_heap_visit_blocks(heap, false, &count_blocks, &allocated_blocks);
}
}
// TODO(sgross): count blocks in abandoned segments.

mi_abandoned_pool_t *pool = &interp->mimalloc.abandoned_pool;
for (uint8_t tag = 0; tag < _Py_MIMALLOC_HEAP_COUNT; tag++) {
_mi_abandoned_pool_visit_blocks(pool, tag, false, &count_blocks,
&allocated_blocks);
}
#else
// TODO(sgross): this only counts the current thread's blocks.
mi_heap_t *heap = mi_heap_get_default();
Expand DownExpand Up@@ -1189,6 +1194,7 @@ get_num_global_allocated_blocks(_PyRuntimeState *runtime)
}
}
else {
_PyEval_StopTheWorldAll(&_PyRuntime);
HEAD_LOCK(runtime);
PyInterpreterState *interp = PyInterpreterState_Head();
assert(interp != NULL);
Expand All@@ -1208,6 +1214,7 @@ get_num_global_allocated_blocks(_PyRuntimeState *runtime)
}
}
HEAD_UNLOCK(runtime);
_PyEval_StartTheWorldAll(&_PyRuntime);
#ifdef Py_DEBUG
assert(got_main);
#endif
Expand Down