From 2984ff9e5196aa575c7322c2040d55dd4d4eaa02 Mon Sep 17 00:00:00 2001 From: Dino Viehland Date: Fri, 28 Mar 2025 15:16:41 -0700 Subject: [PATCH] gh-130373: Avoid locking in _LOAD_ATTR_WITH_HINT (#130372) Avoid locking in _LOAD_ATTR_WITH_HINT --- Include/internal/pycore_dict.h | 4 ++++ Objects/dictobject.c | 6 +++++ Python/bytecodes.c | 28 +++++++++++++----------- Python/executor_cases.c.h | 37 +++++++++++++++++-------------- Python/generated_cases.c.h | 40 +++++++++++++++++++--------------- Python/specialize.c | 3 +++ 6 files changed, 72 insertions(+), 46 deletions(-) diff --git a/Include/internal/pycore_dict.h b/Include/internal/pycore_dict.h index 973772a262e9..754eb88a85c3 100644 --- a/Include/internal/pycore_dict.h +++ b/Include/internal/pycore_dict.h @@ -150,6 +150,10 @@ extern int _PyDict_Pop_KnownHash( Py_hash_t hash, PyObject **result); +#ifdef Py_GIL_DISABLED +PyAPI_FUNC(void) _PyDict_EnsureSharedOnRead(PyDictObject *mp); +#endif + #define DKIX_EMPTY (-1) #define DKIX_DUMMY (-2) /* Used internally */ #define DKIX_ERROR (-3) diff --git a/Objects/dictobject.c b/Objects/dictobject.c index 25ad525c7762..a290eae6464e 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -1326,6 +1326,12 @@ ensure_shared_on_read(PyDictObject *mp) Py_END_CRITICAL_SECTION(); } } + +void +_PyDict_EnsureSharedOnRead(PyDictObject *mp) +{ + ensure_shared_on_read(mp); +} #endif static inline void diff --git a/Python/bytecodes.c b/Python/bytecodes.c index b3b7441c3156..63367141c695 100644 --- a/Python/bytecodes.c +++ b/Python/bytecodes.c @@ -2283,34 +2283,36 @@ dummy_func( assert(Py_TYPE(owner_o)->tp_flags & Py_TPFLAGS_MANAGED_DICT); PyDictObject *dict = _PyObject_GetManagedDict(owner_o); DEOPT_IF(dict == NULL); + PyDictKeysObject *dk = FT_ATOMIC_LOAD_PTR(dict->ma_keys); assert(PyDict_CheckExact((PyObject *)dict)); +#ifdef Py_GIL_DISABLED + DEOPT_IF(!_Py_IsOwnedByCurrentThread((PyObject *)dict) && !_PyObject_GC_IS_SHARED(dict)); +#endif PyObject *attr_o; - if (!LOCK_OBJECT(dict)) { + if (hint >= (size_t)FT_ATOMIC_LOAD_SSIZE_RELAXED(dk->dk_nentries)) { DEOPT_IF(true); } - if (hint >= (size_t)dict->ma_keys->dk_nentries) { - UNLOCK_OBJECT(dict); - DEOPT_IF(true); - } PyObject *name = GETITEM(FRAME_CO_NAMES, oparg>>1); - if (dict->ma_keys->dk_kind != DICT_KEYS_UNICODE) { - UNLOCK_OBJECT(dict); + if (dk->dk_kind != DICT_KEYS_UNICODE) { DEOPT_IF(true); } - PyDictUnicodeEntry *ep = DK_UNICODE_ENTRIES(dict->ma_keys) + hint; - if (ep->me_key != name) { - UNLOCK_OBJECT(dict); + PyDictUnicodeEntry *ep = DK_UNICODE_ENTRIES(dk) + hint; + if (FT_ATOMIC_LOAD_PTR_RELAXED(ep->me_key) != name) { DEOPT_IF(true); } - attr_o = ep->me_value; + attr_o = FT_ATOMIC_LOAD_PTR(ep->me_value); if (attr_o == NULL) { - UNLOCK_OBJECT(dict); DEOPT_IF(true); } STAT_INC(LOAD_ATTR, hit); +#ifdef Py_GIL_DISABLED + if (!_Py_TryIncrefCompareStackRef(&ep->me_value, attr_o, &attr)) { + DEOPT_IF(true); + } +#else attr = PyStackRef_FromPyObjectNew(attr_o); - UNLOCK_OBJECT(dict); +#endif PyStackRef_CLOSE(owner); } diff --git a/Python/executor_cases.c.h b/Python/executor_cases.c.h index 9306a6aea354..bf21c7bfcef3 100644 --- a/Python/executor_cases.c.h +++ b/Python/executor_cases.c.h @@ -3209,48 +3209,53 @@ UOP_STAT_INC(uopcode, miss); JUMP_TO_JUMP_TARGET(); } + PyDictKeysObject *dk = FT_ATOMIC_LOAD_PTR(dict->ma_keys); assert(PyDict_CheckExact((PyObject *)dict)); - PyObject *attr_o; - if (!LOCK_OBJECT(dict)) { - if (true) { - UOP_STAT_INC(uopcode, miss); - JUMP_TO_JUMP_TARGET(); - } + #ifdef Py_GIL_DISABLED + if (!_Py_IsOwnedByCurrentThread((PyObject *)dict) && !_PyObject_GC_IS_SHARED(dict)) { + UOP_STAT_INC(uopcode, miss); + JUMP_TO_JUMP_TARGET(); } - if (hint >= (size_t)dict->ma_keys->dk_nentries) { - UNLOCK_OBJECT(dict); + #endif + PyObject *attr_o; + if (hint >= (size_t)FT_ATOMIC_LOAD_SSIZE_RELAXED(dk->dk_nentries)) { if (true) { UOP_STAT_INC(uopcode, miss); JUMP_TO_JUMP_TARGET(); } } PyObject *name = GETITEM(FRAME_CO_NAMES, oparg>>1); - if (dict->ma_keys->dk_kind != DICT_KEYS_UNICODE) { - UNLOCK_OBJECT(dict); + if (dk->dk_kind != DICT_KEYS_UNICODE) { if (true) { UOP_STAT_INC(uopcode, miss); JUMP_TO_JUMP_TARGET(); } } - PyDictUnicodeEntry *ep = DK_UNICODE_ENTRIES(dict->ma_keys) + hint; - if (ep->me_key != name) { - UNLOCK_OBJECT(dict); + PyDictUnicodeEntry *ep = DK_UNICODE_ENTRIES(dk) + hint; + if (FT_ATOMIC_LOAD_PTR_RELAXED(ep->me_key) != name) { if (true) { UOP_STAT_INC(uopcode, miss); JUMP_TO_JUMP_TARGET(); } } - attr_o = ep->me_value; + attr_o = FT_ATOMIC_LOAD_PTR(ep->me_value); if (attr_o == NULL) { - UNLOCK_OBJECT(dict); if (true) { UOP_STAT_INC(uopcode, miss); JUMP_TO_JUMP_TARGET(); } } STAT_INC(LOAD_ATTR, hit); + #ifdef Py_GIL_DISABLED + if (!_Py_TryIncrefCompareStackRef(&ep->me_value, attr_o, &attr)) { + if (true) { + UOP_STAT_INC(uopcode, miss); + JUMP_TO_JUMP_TARGET(); + } + } + #else attr = PyStackRef_FromPyObjectNew(attr_o); - UNLOCK_OBJECT(dict); + #endif stack_pointer[-1] = attr; _PyFrame_SetStackPointer(frame, stack_pointer); PyStackRef_CLOSE(owner); diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h index f1e22f6d3dd7..4e37c68e2bd8 100644 --- a/Python/generated_cases.c.h +++ b/Python/generated_cases.c.h @@ -8618,17 +8618,17 @@ assert(_PyOpcode_Deopt[opcode] == (LOAD_ATTR)); JUMP_TO_PREDICTED(LOAD_ATTR); } + PyDictKeysObject *dk = FT_ATOMIC_LOAD_PTR(dict->ma_keys); assert(PyDict_CheckExact((PyObject *)dict)); - PyObject *attr_o; - if (!LOCK_OBJECT(dict)) { - if (true) { - UPDATE_MISS_STATS(LOAD_ATTR); - assert(_PyOpcode_Deopt[opcode] == (LOAD_ATTR)); - JUMP_TO_PREDICTED(LOAD_ATTR); - } + #ifdef Py_GIL_DISABLED + if (!_Py_IsOwnedByCurrentThread((PyObject *)dict) && !_PyObject_GC_IS_SHARED(dict)) { + UPDATE_MISS_STATS(LOAD_ATTR); + assert(_PyOpcode_Deopt[opcode] == (LOAD_ATTR)); + JUMP_TO_PREDICTED(LOAD_ATTR); } - if (hint >= (size_t)dict->ma_keys->dk_nentries) { - UNLOCK_OBJECT(dict); + #endif + PyObject *attr_o; + if (hint >= (size_t)FT_ATOMIC_LOAD_SSIZE_RELAXED(dk->dk_nentries)) { if (true) { UPDATE_MISS_STATS(LOAD_ATTR); assert(_PyOpcode_Deopt[opcode] == (LOAD_ATTR)); @@ -8636,26 +8636,23 @@ } } PyObject *name = GETITEM(FRAME_CO_NAMES, oparg>>1); - if (dict->ma_keys->dk_kind != DICT_KEYS_UNICODE) { - UNLOCK_OBJECT(dict); + if (dk->dk_kind != DICT_KEYS_UNICODE) { if (true) { UPDATE_MISS_STATS(LOAD_ATTR); assert(_PyOpcode_Deopt[opcode] == (LOAD_ATTR)); JUMP_TO_PREDICTED(LOAD_ATTR); } } - PyDictUnicodeEntry *ep = DK_UNICODE_ENTRIES(dict->ma_keys) + hint; - if (ep->me_key != name) { - UNLOCK_OBJECT(dict); + PyDictUnicodeEntry *ep = DK_UNICODE_ENTRIES(dk) + hint; + if (FT_ATOMIC_LOAD_PTR_RELAXED(ep->me_key) != name) { if (true) { UPDATE_MISS_STATS(LOAD_ATTR); assert(_PyOpcode_Deopt[opcode] == (LOAD_ATTR)); JUMP_TO_PREDICTED(LOAD_ATTR); } } - attr_o = ep->me_value; + attr_o = FT_ATOMIC_LOAD_PTR(ep->me_value); if (attr_o == NULL) { - UNLOCK_OBJECT(dict); if (true) { UPDATE_MISS_STATS(LOAD_ATTR); assert(_PyOpcode_Deopt[opcode] == (LOAD_ATTR)); @@ -8663,8 +8660,17 @@ } } STAT_INC(LOAD_ATTR, hit); + #ifdef Py_GIL_DISABLED + if (!_Py_TryIncrefCompareStackRef(&ep->me_value, attr_o, &attr)) { + if (true) { + UPDATE_MISS_STATS(LOAD_ATTR); + assert(_PyOpcode_Deopt[opcode] == (LOAD_ATTR)); + JUMP_TO_PREDICTED(LOAD_ATTR); + } + } + #else attr = PyStackRef_FromPyObjectNew(attr_o); - UNLOCK_OBJECT(dict); + #endif stack_pointer[-1] = attr; _PyFrame_SetStackPointer(frame, stack_pointer); PyStackRef_CLOSE(owner); diff --git a/Python/specialize.c b/Python/specialize.c index 5634c601bc5d..ac847b7c752b 100644 --- a/Python/specialize.c +++ b/Python/specialize.c @@ -1009,6 +1009,9 @@ specialize_dict_access_hint( _PyAttrCache *cache = (_PyAttrCache *)(instr + 1); _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(dict); +#ifdef Py_GIL_DISABLED + _PyDict_EnsureSharedOnRead(dict); +#endif // We found an instance with a __dict__. if (_PyDict_HasSplitTable(dict)) {