Skip to content

gh-124218: Avoid refcount contention on builtins module #125847

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

Merged
merged 2 commits into from
Oct 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 0 additions & 3 deletions Include/internal/pycore_ceval.h
Original file line number Diff line number Diff line change
Expand Up @@ -83,9 +83,6 @@ extern void _PyEval_Fini(void);


extern PyObject* _PyEval_GetBuiltins(PyThreadState *tstate);
extern PyObject* _PyEval_BuiltinsFromGlobals(
PyThreadState *tstate,
PyObject *globals);

// Trampoline API

Expand Down
29 changes: 29 additions & 0 deletions Include/internal/pycore_dict.h
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,9 @@ extern Py_ssize_t _PyDictKeys_StringLookup(PyDictKeysObject* dictkeys, PyObject
PyAPI_FUNC(PyObject *)_PyDict_LoadGlobal(PyDictObject *, PyDictObject *, PyObject *);
PyAPI_FUNC(void) _PyDict_LoadGlobalStackRef(PyDictObject *, PyDictObject *, PyObject *, _PyStackRef *);

// Loads the __builtins__ object from the globals dict. Returns a new reference.
extern PyObject *_PyDict_LoadBuiltinsFromGlobals(PyObject *globals);

/* Consumes references to key and value */
PyAPI_FUNC(int) _PyDict_SetItem_Take2(PyDictObject *op, PyObject *key, PyObject *value);
extern int _PyDict_SetItem_LockHeld(PyDictObject *dict, PyObject *name, PyObject *value);
Expand Down Expand Up @@ -318,6 +321,8 @@ PyDictObject *_PyObject_MaterializeManagedDict_LockHeld(PyObject *);
#ifndef Py_GIL_DISABLED
# define _Py_INCREF_DICT Py_INCREF
# define _Py_DECREF_DICT Py_DECREF
# define _Py_INCREF_BUILTINS Py_INCREF
# define _Py_DECREF_BUILTINS Py_DECREF
#else
static inline Py_ssize_t
_PyDict_UniqueId(PyDictObject *mp)
Expand All @@ -341,6 +346,30 @@ _Py_DECREF_DICT(PyObject *op)
Py_ssize_t id = _PyDict_UniqueId((PyDictObject *)op);
_Py_THREAD_DECREF_OBJECT(op, id);
}

// Like `_Py_INCREF_DICT`, but also handles non-dict objects because builtins
// may not be a dict.
static inline void
_Py_INCREF_BUILTINS(PyObject *op)
{
if (PyDict_CheckExact(op)) {
_Py_INCREF_DICT(op);
}
else {
Py_INCREF(op);
}
}

static inline void
_Py_DECREF_BUILTINS(PyObject *op)
{
if (PyDict_CheckExact(op)) {
_Py_DECREF_DICT(op);
}
else {
Py_DECREF(op);
}
}
#endif

#ifdef __cplusplus
Expand Down
34 changes: 34 additions & 0 deletions Objects/dictobject.c
Original file line number Diff line number Diff line change
Expand Up @@ -2511,6 +2511,40 @@ _PyDict_LoadGlobalStackRef(PyDictObject *globals, PyDictObject *builtins, PyObje
assert(ix >= 0 || PyStackRef_IsNull(*res));
}

PyObject *
_PyDict_LoadBuiltinsFromGlobals(PyObject *globals)
{
if (!PyDict_Check(globals)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe just add assertion as it is internal only API

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some of the callers can pass in a non-dict "globals". IIRC, test_eval_code_ex.py tests for this case and expects a SystemError.

PyErr_BadInternalCall();
return NULL;
}

PyDictObject *mp = (PyDictObject *)globals;
PyObject *key = &_Py_ID(__builtins__);
Py_hash_t hash = unicode_get_hash(key);

// Use the stackref variant to avoid reference count contention on the
// builtins module in the free threading build. It's important not to
// make any escaping calls between the lookup and the `PyStackRef_CLOSE()`
// because the `ref` is not visible to the GC.
_PyStackRef ref;
Py_ssize_t ix = _Py_dict_lookup_threadsafe_stackref(mp, key, hash, &ref);
if (ix == DKIX_ERROR) {
return NULL;
}
if (PyStackRef_IsNull(ref)) {
return Py_NewRef(PyEval_GetBuiltins());
}
PyObject *builtins = PyStackRef_AsPyObjectBorrow(ref);
if (PyModule_Check(builtins)) {
builtins = _PyModule_GetDict(builtins);
assert(builtins != NULL);
}
_Py_INCREF_BUILTINS(builtins);
PyStackRef_CLOSE(ref);
return builtins;
}

/* Consumes references to key and value */
static int
setitem_take2_lock_held(PyDictObject *mp, PyObject *key, PyObject *value)
Expand Down
24 changes: 4 additions & 20 deletions Objects/frameobject.c
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
/* Frame object implementation */

#include "Python.h"
#include "pycore_ceval.h" // _PyEval_BuiltinsFromGlobals()
#include "pycore_ceval.h" // _PyEval_SetOpcodeTrace()
#include "pycore_code.h" // CO_FAST_LOCAL, etc.
#include "pycore_dict.h" // _PyDict_LoadBuiltinsFromGlobals()
#include "pycore_function.h" // _PyFunction_FromConstructor()
#include "pycore_moduleobject.h" // _PyModule_GetDict()
#include "pycore_modsupport.h" // _PyArg_CheckPositional()
Expand Down Expand Up @@ -1899,7 +1900,7 @@ PyFrameObject*
PyFrame_New(PyThreadState *tstate, PyCodeObject *code,
PyObject *globals, PyObject *locals)
{
PyObject *builtins = _PyEval_BuiltinsFromGlobals(tstate, globals); // borrowed ref
PyObject *builtins = _PyDict_LoadBuiltinsFromGlobals(globals);
if (builtins == NULL) {
return NULL;
}
Expand All @@ -1914,6 +1915,7 @@ PyFrame_New(PyThreadState *tstate, PyCodeObject *code,
.fc_closure = NULL
};
PyFunctionObject *func = _PyFunction_FromConstructor(&desc);
_Py_DECREF_BUILTINS(builtins);
if (func == NULL) {
return NULL;
}
Expand Down Expand Up @@ -2204,21 +2206,3 @@ PyFrame_GetGenerator(PyFrameObject *frame)
PyGenObject *gen = _PyGen_GetGeneratorFromFrame(frame->f_frame);
return Py_NewRef(gen);
}

PyObject*
_PyEval_BuiltinsFromGlobals(PyThreadState *tstate, PyObject *globals)
{
PyObject *builtins = PyDict_GetItemWithError(globals, &_Py_ID(__builtins__));
if (builtins) {
if (PyModule_Check(builtins)) {
builtins = _PyModule_GetDict(builtins);
assert(builtins != NULL);
}
return builtins;
}
if (PyErr_Occurred()) {
return NULL;
}

return _PyEval_GetBuiltins(tstate);
}
25 changes: 3 additions & 22 deletions Objects/funcobject.c
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
/* Function object implementation */

#include "Python.h"
#include "pycore_ceval.h" // _PyEval_BuiltinsFromGlobals()
#include "pycore_dict.h" // _Py_INCREF_DICT()
#include "pycore_long.h" // _PyLong_GetOne()
#include "pycore_modsupport.h" // _PyArg_NoKeywords()
Expand Down Expand Up @@ -115,12 +114,7 @@ _PyFunction_FromConstructor(PyFrameConstructor *constr)
}
_Py_INCREF_DICT(constr->fc_globals);
op->func_globals = constr->fc_globals;
if (PyDict_Check(constr->fc_builtins)) {
_Py_INCREF_DICT(constr->fc_builtins);
}
else {
Py_INCREF(constr->fc_builtins);
}
_Py_INCREF_BUILTINS(constr->fc_builtins);
op->func_builtins = constr->fc_builtins;
op->func_name = Py_NewRef(constr->fc_name);
op->func_qualname = Py_NewRef(constr->fc_qualname);
Expand Down Expand Up @@ -153,8 +147,6 @@ PyFunction_NewWithQualName(PyObject *code, PyObject *globals, PyObject *qualname
assert(PyDict_Check(globals));
_Py_INCREF_DICT(globals);

PyThreadState *tstate = _PyThreadState_GET();

PyCodeObject *code_obj = (PyCodeObject *)code;
_Py_INCREF_CODE(code_obj);

Expand Down Expand Up @@ -188,16 +180,10 @@ PyFunction_NewWithQualName(PyObject *code, PyObject *globals, PyObject *qualname
goto error;
}

builtins = _PyEval_BuiltinsFromGlobals(tstate, globals); // borrowed ref
builtins = _PyDict_LoadBuiltinsFromGlobals(globals);
if (builtins == NULL) {
goto error;
}
if (PyDict_Check(builtins)) {
_Py_INCREF_DICT(builtins);
}
else {
Py_INCREF(builtins);
}

PyFunctionObject *op = PyObject_GC_New(PyFunctionObject, &PyFunction_Type);
if (op == NULL) {
Expand Down Expand Up @@ -1078,12 +1064,7 @@ func_clear(PyObject *self)
PyObject *builtins = op->func_builtins;
op->func_builtins = NULL;
if (builtins != NULL) {
if (PyDict_Check(builtins)) {
_Py_DECREF_DICT(builtins);
}
else {
Py_DECREF(builtins);
}
_Py_DECREF_BUILTINS(builtins);
}
Py_CLEAR(op->func_module);
Py_CLEAR(op->func_defaults);
Expand Down
6 changes: 4 additions & 2 deletions Python/ceval.c
Original file line number Diff line number Diff line change
Expand Up @@ -639,7 +639,7 @@ PyEval_EvalCode(PyObject *co, PyObject *globals, PyObject *locals)
if (locals == NULL) {
locals = globals;
}
PyObject *builtins = _PyEval_BuiltinsFromGlobals(tstate, globals); // borrowed ref
PyObject *builtins = _PyDict_LoadBuiltinsFromGlobals(globals);
if (builtins == NULL) {
return NULL;
}
Expand All @@ -654,6 +654,7 @@ PyEval_EvalCode(PyObject *co, PyObject *globals, PyObject *locals)
.fc_closure = NULL
};
PyFunctionObject *func = _PyFunction_FromConstructor(&desc);
_Py_DECREF_BUILTINS(builtins);
if (func == NULL) {
return NULL;
}
Expand Down Expand Up @@ -1899,7 +1900,7 @@ PyEval_EvalCodeEx(PyObject *_co, PyObject *globals, PyObject *locals,
if (defaults == NULL) {
return NULL;
}
PyObject *builtins = _PyEval_BuiltinsFromGlobals(tstate, globals); // borrowed ref
PyObject *builtins = _PyDict_LoadBuiltinsFromGlobals(globals);
if (builtins == NULL) {
Py_DECREF(defaults);
return NULL;
Expand Down Expand Up @@ -1954,6 +1955,7 @@ PyEval_EvalCodeEx(PyObject *_co, PyObject *globals, PyObject *locals,
Py_XDECREF(func);
Py_XDECREF(kwnames);
PyMem_Free(newargs);
_Py_DECREF_BUILTINS(builtins);
Py_DECREF(defaults);
return res;
}
Expand Down
Loading