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 biased reference counting #110764

Merged
merged 8 commits into from
Oct 30, 2023
Merged

gh-110481: Implement biased reference counting #110764

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
94bfbde
Remove trailing commas in _PyObject_HEAD_INIT and _PyVarObject_HEAD_INIT
colesbury Oct 4, 2023
b1cb396
Implement biased reference counting
colesbury Sep 13, 2023
425d570
Add blurb
colesbury Oct 12, 2023
8c68c78
Changes from review:
colesbury Oct 19, 2023
86892d9
Fix compile errors from rebasing on main
colesbury Oct 19, 2023
3c85ca1
Fix typo
colesbury Oct 20, 2023
c6c6ec6
Add missing guards
colesbury Oct 20, 2023
ac2c957
Add TODOs for missing Py_DECREF specializations
colesbury Oct 24, 2023
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 Include/internal/pycore_long.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,7 +317,7 @@ _PyLong_FlipSign(PyLongObject *op) {

#define _PyLong_DIGIT_INIT(val) \
{ \
.ob_base = _PyObject_HEAD_INIT(&PyLong_Type) \
.ob_base = _PyObject_HEAD_INIT(&PyLong_Type), \
.long_value = { \
.lv_tag = TAG_FROM_SIGN_AND_SIZE( \
(val) == 0 ? 0 : ((val) < 0 ? -1 : 1), \
Expand Down
89 changes: 84 additions & 5 deletions Include/internal/pycore_object.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,16 +54,24 @@ PyAPI_FUNC(int) _PyObject_IsFreed(PyObject *);
Furthermore, we can't use designated initializers in Extensions since these
are not supported pre-C++20. Thus, keeping an internal copy here is the most
backwards compatible solution */
#if defined(Py_NOGIL)
#define _PyObject_HEAD_INIT(type) \
{ \
.ob_ref_local = _Py_IMMORTAL_REFCNT_LOCAL, \
.ob_type = (type) \
}
#else
#define _PyObject_HEAD_INIT(type) \
{ \
.ob_refcnt = _Py_IMMORTAL_REFCNT, \
.ob_type = (type) \
},
}
Copy link
Member

Choose a reason for hiding this comment

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

Loosing the trailing comma is definitely better style, but is bound to break some third party code.
I think you need to leave the comma.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is in an internal-only header (Include/internal/pycore_object.h), not used by third party code.

There's a similarly named public macro used by third party code that is not changed.

#endif
#define _PyVarObject_HEAD_INIT(type, size) \
{ \
.ob_base = _PyObject_HEAD_INIT(type) \
.ob_base = _PyObject_HEAD_INIT(type), \
.ob_size = size \
},
}
corona10 marked this conversation as resolved.
Show resolved Hide resolved

extern void _Py_NO_RETURN _Py_FatalRefcountErrorFunc(
const char *func,
Expand Down Expand Up @@ -95,24 +103,63 @@ static inline void _Py_RefcntAdd(PyObject* op, Py_ssize_t n)
#ifdef Py_REF_DEBUG
_Py_AddRefTotal(_PyInterpreterState_GET(), n);
#endif
#if !defined(Py_NOGIL)
Copy link
Member

Choose a reason for hiding this comment

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

Is this worth the extra code?
_Py_RefcntAdd is only used in list_repeat and tuplerepeat. Multiplying tuples and lists by ints is hardly a common operation.
Would it perhaps make more sense to remove _Py_RefcntAdd and use plain Py_INCREF in those functions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you suggesting removing _Py_RefcntAdd in general or just for Py_NOGIL?

op->ob_refcnt += n;
#else
if (_Py_IsOwnedByCurrentThread(op)) {
uint32_t local = op->ob_ref_local;
Py_ssize_t refcnt = (Py_ssize_t)local + n;
# if PY_SSIZE_T_MAX > UINT32_MAX
if (refcnt > (Py_ssize_t)UINT32_MAX) {
// Make the object immortal if the 32-bit local reference count
// would overflow.
refcnt = _Py_IMMORTAL_REFCNT_LOCAL;
}
# endif
_Py_atomic_store_uint32_relaxed(&op->ob_ref_local, (uint32_t)refcnt);
}
else {
_Py_atomic_add_ssize(&op->ob_ref_shared, (n << _Py_REF_SHARED_SHIFT));
}
#endif
}
#define _Py_RefcntAdd(op, n) _Py_RefcntAdd(_PyObject_CAST(op), n)

static inline void _Py_SetImmortal(PyObject *op)
{
if (op) {
#ifdef Py_NOGIL
op->ob_tid = _Py_UNOWNED_TID;
op->ob_ref_local = _Py_IMMORTAL_REFCNT_LOCAL;
op->ob_ref_shared = 0;
#else
op->ob_refcnt = _Py_IMMORTAL_REFCNT;
#endif
}
}
#define _Py_SetImmortal(op) _Py_SetImmortal(_PyObject_CAST(op))

// Makes an immortal object mortal again with the specified refcnt. Should only
// be used during runtime finalization.
static inline void _Py_SetMortal(PyObject *op, Py_ssize_t refcnt)
Copy link
Member

Choose a reason for hiding this comment

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

This function shouldn't exist.
Making an immortal object mortal is not safe.
If it is used during runtime finalization, then the finalization should be fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is important for the reference leak checks. It's not new functionality. Without it, we would leak immortal objects on interpreter shutdown. It's not generally safe, which is why it's in an Include/internal header.

We use this in two places. In both our intention is to deallocate immortal objects.

  1. In _PyUnicode_ClearInterned

    cpython/Objects/unicodeobject.c

    Lines 14967 to 14970 in 3726cb0

    // Skip the Immortal Instance check and restore
    // the two references (key and value) ignored
    // by PyUnicode_InternInPlace().
    s->ob_refcnt = 2;
  2. In _Py_ClearImmortal to deallocate immortal objects.

{
if (op) {
assert(_Py_IsImmortal(op));
#ifdef Py_NOGIL
op->ob_tid = _Py_UNOWNED_TID;
op->ob_ref_local = 0;
op->ob_ref_shared = _Py_REF_SHARED(refcnt, _Py_REF_MERGED);
#else
op->ob_refcnt = refcnt;
#endif
}
}

/* _Py_ClearImmortal() should only be used during runtime finalization. */
static inline void _Py_ClearImmortal(PyObject *op)
{
if (op) {
assert(op->ob_refcnt == _Py_IMMORTAL_REFCNT);
op->ob_refcnt = 1;
_Py_SetMortal(op, 1);
Py_DECREF(op);
}
}
Expand All @@ -122,6 +169,7 @@ static inline void _Py_ClearImmortal(PyObject *op)
op = NULL; \
} while (0)

#if !defined(Py_NOGIL)
static inline void
_Py_DECREF_SPECIALIZED(PyObject *op, const destructor destruct)
{
Expand Down Expand Up @@ -161,6 +209,37 @@ _Py_DECREF_NO_DEALLOC(PyObject *op)
#endif
}

#else
// TODO: implement Py_DECREF specializations for Py_NOGIL build
static inline void
_Py_DECREF_SPECIALIZED(PyObject *op, const destructor destruct)
Copy link
Member

Choose a reason for hiding this comment

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

Why not keep the specialization?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm trying to keep the PR small so that it's easier to review. I intend to implement the specializations later.

{
Py_DECREF(op);
}

static inline void
_Py_DECREF_NO_DEALLOC(PyObject *op)
Copy link
Member

Choose a reason for hiding this comment

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

Given we know that the refcount cannot reach zero, why not take advantage of that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I intend to implement this later.

{
Py_DECREF(op);
}

static inline int
_Py_REF_IS_MERGED(Py_ssize_t ob_ref_shared)
{
return (ob_ref_shared & _Py_REF_SHARED_FLAG_MASK) == _Py_REF_MERGED;
}

static inline int
_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
# undef _Py_DEC_REFTOTAL
#endif
Expand Down
14 changes: 7 additions & 7 deletions Include/internal/pycore_runtime_init.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,13 +129,13 @@ extern PyTypeObject _PyExc_MemoryError;
.latin1 = _Py_str_latin1_INIT, \
}, \
.tuple_empty = { \
.ob_base = _PyVarObject_HEAD_INIT(&PyTuple_Type, 0) \
.ob_base = _PyVarObject_HEAD_INIT(&PyTuple_Type, 0), \
}, \
.hamt_bitmap_node_empty = { \
.ob_base = _PyVarObject_HEAD_INIT(&_PyHamt_BitmapNode_Type, 0) \
.ob_base = _PyVarObject_HEAD_INIT(&_PyHamt_BitmapNode_Type, 0), \
}, \
.context_token_missing = { \
.ob_base = _PyObject_HEAD_INIT(&_PyContextTokenMissing_Type) \
.ob_base = _PyObject_HEAD_INIT(&_PyContextTokenMissing_Type), \
}, \
}, \
}, \
Expand Down Expand Up @@ -172,11 +172,11 @@ extern PyTypeObject _PyExc_MemoryError;
.singletons = { \
._not_used = 1, \
.hamt_empty = { \
.ob_base = _PyObject_HEAD_INIT(&_PyHamt_Type) \
.ob_base = _PyObject_HEAD_INIT(&_PyHamt_Type), \
.h_root = (PyHamtNode*)&_Py_SINGLETON(hamt_bitmap_node_empty), \
}, \
.last_resort_memory_error = { \
_PyObject_HEAD_INIT(&_PyExc_MemoryError) \
_PyObject_HEAD_INIT(&_PyExc_MemoryError), \
.args = (PyObject*)&_Py_SINGLETON(tuple_empty) \
}, \
}, \
Expand Down Expand Up @@ -206,7 +206,7 @@ extern PyTypeObject _PyExc_MemoryError;

#define _PyBytes_SIMPLE_INIT(CH, LEN) \
{ \
_PyVarObject_HEAD_INIT(&PyBytes_Type, (LEN)) \
_PyVarObject_HEAD_INIT(&PyBytes_Type, (LEN)), \
.ob_shash = -1, \
.ob_sval = { (CH) }, \
}
Expand All @@ -217,7 +217,7 @@ extern PyTypeObject _PyExc_MemoryError;

#define _PyUnicode_ASCII_BASE_INIT(LITERAL, ASCII) \
{ \
.ob_base = _PyObject_HEAD_INIT(&PyUnicode_Type) \
.ob_base = _PyObject_HEAD_INIT(&PyUnicode_Type), \
.length = sizeof(LITERAL) - 1, \
.hash = -1, \
.state = { \
Expand Down
Loading
Loading