From 548a7c275ccf7bfcbe3d50c21ced540b4502d281 Mon Sep 17 00:00:00 2001 From: Dino Viehland Date: Wed, 3 Apr 2024 16:58:59 -0700 Subject: [PATCH 1/2] Make _PyDict_LoadGlobal threadsafe --- Include/internal/pycore_dict.h | 1 + Objects/dictobject.c | 27 +++++++++++++++++++++------ Objects/odictobject.c | 6 +++++- Python/bytecodes.c | 1 - Python/executor_cases.c.h | 1 - Python/generated_cases.c.h | 1 - 6 files changed, 27 insertions(+), 10 deletions(-) diff --git a/Include/internal/pycore_dict.h b/Include/internal/pycore_dict.h index 5507bdd539cc42..fba0dfc40714ec 100644 --- a/Include/internal/pycore_dict.h +++ b/Include/internal/pycore_dict.h @@ -97,6 +97,7 @@ extern void _PyDictKeys_DecRef(PyDictKeysObject *keys); * -1 when no entry found, -3 when compare raises error. */ extern Py_ssize_t _Py_dict_lookup(PyDictObject *mp, PyObject *key, Py_hash_t hash, PyObject **value_addr); +extern Py_ssize_t _Py_dict_lookup_threadsafe(PyDictObject *mp, PyObject *key, Py_hash_t hash, PyObject **value_addr); extern Py_ssize_t _PyDict_LookupIndex(PyDictObject *, PyObject *); extern Py_ssize_t _PyDictKeys_StringLookup(PyDictKeysObject* dictkeys, PyObject *key); diff --git a/Objects/dictobject.c b/Objects/dictobject.c index 58a3d979339c49..87f221b2fefaf5 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -1065,7 +1065,6 @@ compare_unicode_generic(PyDictObject *mp, PyDictKeysObject *dk, assert(ep->me_key != NULL); assert(PyUnicode_CheckExact(ep->me_key)); assert(!PyUnicode_CheckExact(key)); - // TODO: Thread safety if (unicode_get_hash(ep->me_key) == hash) { PyObject *startkey = ep->me_key; @@ -1192,7 +1191,8 @@ _Py_dict_lookup(PyDictObject *mp, PyObject *key, Py_hash_t hash, PyObject **valu PyDictKeysObject *dk; DictKeysKind kind; Py_ssize_t ix; - // TODO: Thread safety + + _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(mp); start: dk = mp->ma_keys; kind = dk->dk_kind; @@ -1390,7 +1390,7 @@ dictkeys_generic_lookup_threadsafe(PyDictObject *mp, PyDictKeysObject* dk, PyObj return do_lookup(mp, dk, key, hash, compare_generic_threadsafe); } -static Py_ssize_t +Py_ssize_t _Py_dict_lookup_threadsafe(PyDictObject *mp, PyObject *key, Py_hash_t hash, PyObject **value_addr) { PyDictKeysObject *dk; @@ -2343,11 +2343,12 @@ _PyDict_GetItemStringWithError(PyObject *v, const char *key) * Raise an exception and return NULL if an error occurred (ex: computing the * key hash failed, key comparison failed, ...). Return NULL if the key doesn't * exist. Return the value if the key exists. + * + * Returns a new reference. */ PyObject * _PyDict_LoadGlobal(PyDictObject *globals, PyDictObject *builtins, PyObject *key) { - // TODO: Thread safety Py_ssize_t ix; Py_hash_t hash; PyObject *value; @@ -2358,17 +2359,31 @@ _PyDict_LoadGlobal(PyDictObject *globals, PyDictObject *builtins, PyObject *key) return NULL; } +#ifdef Py_GIL_DISABLED /* namespace 1: globals */ - ix = _Py_dict_lookup(globals, key, hash, &value); + ix = _Py_dict_lookup_threadsafe(globals, key, hash, &value); if (ix == DKIX_ERROR) return NULL; if (ix != DKIX_EMPTY && value != NULL) return value; /* namespace 2: builtins */ - ix = _Py_dict_lookup(builtins, key, hash, &value); + ix = _Py_dict_lookup_threadsafe(builtins, key, hash, &value); assert(ix >= 0 || value == NULL); return value; +#else + /* namespace 1: globals */ + ix = _Py_dict_lookup(globals, key, hash, &value); + if (ix == DKIX_ERROR) + return NULL; + if (ix != DKIX_EMPTY && value != NULL) + return Py_NewRef(value); + + /* namespace 2: builtins */ + ix = _Py_dict_lookup(builtins, key, hash, &value); + assert(ix >= 0 || value == NULL); + return Py_XNewRef(value); +#endif } /* Consumes references to key and value */ diff --git a/Objects/odictobject.c b/Objects/odictobject.c index 421bc52992d735..53f64fc81e7deb 100644 --- a/Objects/odictobject.c +++ b/Objects/odictobject.c @@ -535,8 +535,12 @@ _odict_get_index_raw(PyODictObject *od, PyObject *key, Py_hash_t hash) PyObject *value = NULL; PyDictKeysObject *keys = ((PyDictObject *)od)->ma_keys; Py_ssize_t ix; - +#ifdef Py_GIL_DISABLED + ix = _Py_dict_lookup_threadsafe((PyDictObject *)od, key, hash, &value); + Py_XDECREF(value); +#else ix = _Py_dict_lookup((PyDictObject *)od, key, hash, &value); +#endif if (ix == DKIX_EMPTY) { return keys->dk_nentries; /* index of new entry */ } diff --git a/Python/bytecodes.c b/Python/bytecodes.c index fa53c969fe361e..83113cb1a03cc3 100644 --- a/Python/bytecodes.c +++ b/Python/bytecodes.c @@ -1426,7 +1426,6 @@ dummy_func( } ERROR_IF(true, error); } - Py_INCREF(res); } else { /* Slow-path if globals or builtins is not a dict */ diff --git a/Python/executor_cases.c.h b/Python/executor_cases.c.h index 98476798fbbbdf..e3643bbc19548e 100644 --- a/Python/executor_cases.c.h +++ b/Python/executor_cases.c.h @@ -1254,7 +1254,6 @@ } if (true) JUMP_TO_ERROR(); } - Py_INCREF(res); } else { /* Slow-path if globals or builtins is not a dict */ diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h index 6ee794a05b51d4..9c252252b3d5b0 100644 --- a/Python/generated_cases.c.h +++ b/Python/generated_cases.c.h @@ -4269,7 +4269,6 @@ } if (true) goto error; } - Py_INCREF(res); } else { /* Slow-path if globals or builtins is not a dict */ From 3dd11824bd74a6ef83c325098f97f1277b1b2a88 Mon Sep 17 00:00:00 2001 From: Dino Viehland Date: Thu, 4 Apr 2024 10:59:39 -0700 Subject: [PATCH 2/2] Add version of _Py_dict_lookup_threadsafe w/ the GIL that does incref --- Objects/dictobject.c | 43 ++++++++++--------------------------------- 1 file changed, 10 insertions(+), 33 deletions(-) diff --git a/Objects/dictobject.c b/Objects/dictobject.c index 87f221b2fefaf5..b62d39ad6c5192 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -1488,6 +1488,16 @@ _Py_dict_lookup_threadsafe(PyDictObject *mp, PyObject *key, Py_hash_t hash, PyOb return ix; } +#else // Py_GIL_DISABLED + +Py_ssize_t +_Py_dict_lookup_threadsafe(PyDictObject *mp, PyObject *key, Py_hash_t hash, PyObject **value_addr) +{ + Py_ssize_t ix = _Py_dict_lookup(mp, key, hash, value_addr); + Py_XNewRef(*value_addr); + return ix; +} + #endif int @@ -2359,7 +2369,6 @@ _PyDict_LoadGlobal(PyDictObject *globals, PyDictObject *builtins, PyObject *key) return NULL; } -#ifdef Py_GIL_DISABLED /* namespace 1: globals */ ix = _Py_dict_lookup_threadsafe(globals, key, hash, &value); if (ix == DKIX_ERROR) @@ -2371,19 +2380,6 @@ _PyDict_LoadGlobal(PyDictObject *globals, PyDictObject *builtins, PyObject *key) ix = _Py_dict_lookup_threadsafe(builtins, key, hash, &value); assert(ix >= 0 || value == NULL); return value; -#else - /* namespace 1: globals */ - ix = _Py_dict_lookup(globals, key, hash, &value); - if (ix == DKIX_ERROR) - return NULL; - if (ix != DKIX_EMPTY && value != NULL) - return Py_NewRef(value); - - /* namespace 2: builtins */ - ix = _Py_dict_lookup(builtins, key, hash, &value); - assert(ix >= 0 || value == NULL); - return Py_XNewRef(value); -#endif } /* Consumes references to key and value */ @@ -3229,11 +3225,7 @@ dict_subscript(PyObject *self, PyObject *key) if (hash == -1) return NULL; } -#ifdef Py_GIL_DISABLED ix = _Py_dict_lookup_threadsafe(mp, key, hash, &value); -#else - ix = _Py_dict_lookup(mp, key, hash, &value); -#endif if (ix == DKIX_ERROR) return NULL; if (ix == DKIX_EMPTY || value == NULL) { @@ -3253,11 +3245,7 @@ dict_subscript(PyObject *self, PyObject *key) _PyErr_SetKeyError(key); return NULL; } -#ifdef Py_GIL_DISABLED return value; -#else - return Py_NewRef(value); -#endif } static int @@ -4124,24 +4112,13 @@ dict_get_impl(PyDictObject *self, PyObject *key, PyObject *default_value) if (hash == -1) return NULL; } -#ifdef Py_GIL_DISABLED ix = _Py_dict_lookup_threadsafe(self, key, hash, &val); -#else - ix = _Py_dict_lookup(self, key, hash, &val); -#endif if (ix == DKIX_ERROR) return NULL; -#ifdef Py_GIL_DISABLED if (ix == DKIX_EMPTY || val == NULL) { val = Py_NewRef(default_value); } return val; -#else - if (ix == DKIX_EMPTY || val == NULL) { - val = default_value; - } - return Py_NewRef(val); -#endif } static int