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.

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

Jump to bottom

gh-110481: Implement _Py_DECREF_NO_DEALLOC for free-threaded build #111560

Closed
wants to merge 4 commits into from
Closed

gh-110481: Implement _Py_DECREF_NO_DEALLOC for free-threaded build #111560

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6c2ffb8
gh-110481: Implement _Py_DECREF_NO_DEALLOC for free-threaded build
corona10 Oct 31, 2023
397f018
Remove assert
corona10 Oct 31, 2023
2c2f4df
Update
corona10 Oct 31, 2023
8166ae0
Update
corona10 Oct 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
48 changes: 44 additions & 4 deletions Include/internal/pycore_object.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,10 @@ _Py_DECREF_NO_DEALLOC(PyObject *op)
}

#else
// Merge the local and shared reference count fields and add `extra` to the
// refcount when merging.
Py_ssize_t _Py_ExplicitMergeRefcount(PyObject *op, Py_ssize_t extra);

// TODO: implement Py_DECREF specializations for Py_NOGIL build
static inline void
_Py_DECREF_SPECIALIZED(PyObject *op, const destructor destruct)
Expand All @@ -220,7 +224,46 @@ _Py_DECREF_SPECIALIZED(PyObject *op, const destructor destruct)
static inline void
_Py_DECREF_NO_DEALLOC(PyObject *op)
{
Py_DECREF(op);
if (_Py_IsImmortal(op)) {
return;
}

if (_Py_IsOwnedByCurrentThread(op)) {
uint32_t refcount = _Py_atomic_load_uint32_relaxed(&op->ob_ref_local);
assert(refcount != 0);
refcount--;
_Py_atomic_store_uint32_relaxed(&op->ob_ref_local, refcount);
if (refcount == 0) {
Copy link
Member Author

@corona10 corona10 Oct 31, 2023 •
edited

Choose a reason for hiding this comment

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

Similar question, do we have to handle zero local refcounting cases from _Py_DECREF_NO_DEALLOC
or it can be handled as deferred merging from somewhere?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, you need _Py_MergeZeroLocalRefcount. You can't defer it -- that would break a bunch of invariants. For example, the same thread may try calling Py_DECREF again leading to a negative local refcount.

// Assume that local + share >= 1
_Py_MergeZeroLocalRefcount(op);
}
}
else {
Py_ssize_t refcount = _Py_atomic_load_ssize_relaxed(&op->ob_ref_shared);
Py_ssize_t new_shared;
// Shared refcount can be zero but we should consider local refcount.
int should_queue = (refcount == 0 || refcount == _Py_REF_MAYBE_WEAKREF);
Copy link
Member Author

Choose a reason for hiding this comment

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

@colesbury Out of curiosity, Don't we have to consider that shared_refcount can be a negative value at this moment due to imbalance refcounting?

Same question for the

cpython/Objects/object.c

Line 319 in 2445673

should_queue = (shared == 0 || shared == _Py_REF_MAYBE_WEAKREF);

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I fully understand your question. Yes, shared_refcount may be negative. should_queue is always false if shared is negative because:

  1. we only queue objects once
  2. we queue them the first time the shared refcount becomes negative

So if it's already negative then we must have already queued it.

Copy link
Member Author

Choose a reason for hiding this comment

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

So if it's already negative then we must have already queued it.

Make sense, thank you for explain.

do {
if (should_queue) {
new_shared = _Py_REF_QUEUED;
}
else {
new_shared = refcount - (1 << _Py_REF_SHARED_SHIFT);
}
} while (!_Py_atomic_compare_exchange_ssize(&op->ob_ref_shared,
&refcount, new_shared));

if (should_queue) {
// TODO: the inter-thread queue is not yet implemented. For now,
// we just merge the refcount here.
refcount = _Py_ExplicitMergeRefcount(op, -1);
#ifdef Py_REF_DEBUG
if (refcount <= 0) {
_Py_FatalRefcountError("Expected a positive remaining refcount");
}
#endif
}
}
}

static inline int
Expand All @@ -235,9 +278,6 @@ _Py_REF_IS_QUEUED(Py_ssize_t ob_ref_shared)
return (ob_ref_shared & _Py_REF_SHARED_FLAG_MASK) == _Py_REF_QUEUED;
}

// Merge the local and shared reference count fields and add `extra` to the
// refcount when merging.
Py_ssize_t _Py_ExplicitMergeRefcount(PyObject *op, Py_ssize_t extra);
#endif // !defined(Py_NOGIL)

#ifdef Py_REF_DEBUG
Expand Down