From be5fa22a54e95178cadc53a3df8e3729a0638580 Mon Sep 17 00:00:00 2001 From: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com> Date: Sun, 7 Jul 2024 19:37:48 +0800 Subject: [PATCH 1/7] Make LOAD_ATTR deferred --- Include/internal/pycore_dict.h | 1 + Include/internal/pycore_object.h | 3 + Include/internal/pycore_stackref.h | 3 +- Objects/dictobject.c | 21 +++++ Objects/object.c | 100 ++++++++++++++++++++ Objects/typeobject.c | 141 +++++++++++++++++++++++++++++ Python/bytecodes.c | 17 ++-- Python/executor_cases.c.h | 19 ++-- Python/generated_cases.c.h | 19 ++-- 9 files changed, 290 insertions(+), 34 deletions(-) diff --git a/Include/internal/pycore_dict.h b/Include/internal/pycore_dict.h index f9a043b0208c8f..53bdc71a9bb1d7 100644 --- a/Include/internal/pycore_dict.h +++ b/Include/internal/pycore_dict.h @@ -118,6 +118,7 @@ PyAPI_FUNC(int) _PyDict_SetItem_KnownHash_LockHeld(PyDictObject *mp, PyObject *k PyAPI_FUNC(int) _PyDict_GetItemRef_KnownHash_LockHeld(PyDictObject *op, PyObject *key, Py_hash_t hash, PyObject **result); extern int _PyDict_GetItemRef_KnownHash(PyDictObject *op, PyObject *key, Py_hash_t hash, PyObject **result); extern int _PyDict_GetItemRef_Unicode_LockHeld(PyDictObject *op, PyObject *key, PyObject **result); +extern int _PyDict_GetItem_KnownHash_StackRef(PyDictObject *op, PyObject *key, Py_hash_t hash, _PyStackRef *result); extern int _PyObjectDict_SetItem(PyTypeObject *tp, PyObject *obj, PyObject **dictptr, PyObject *name, PyObject *value); extern int _PyDict_Pop_KnownHash( diff --git a/Include/internal/pycore_object.h b/Include/internal/pycore_object.h index 64ff44bd7f5e43..5f9084bc1b14a7 100644 --- a/Include/internal/pycore_object.h +++ b/Include/internal/pycore_object.h @@ -837,6 +837,7 @@ PyAPI_FUNC(PyObject*) _PyObject_LookupSpecialMethod(PyObject *self, PyObject *at extern int _PyObject_IsAbstract(PyObject *); PyAPI_FUNC(int) _PyObject_GetMethod(PyObject *obj, PyObject *name, PyObject **method); +PyAPI_FUNC(int) _PyObject_GetMethodStackRef(PyObject *obj, PyObject *name, _PyStackRef *method); extern PyObject* _PyObject_NextNotImplemented(PyObject *); // Pickle support. @@ -874,6 +875,8 @@ PyAPI_DATA(int) _Py_SwappedOp[]; extern void _Py_GetConstant_Init(void); +extern void _PyType_LookupStackRef(PyTypeObject *type, PyObject *name, _PyStackRef *result); + #ifdef __cplusplus } #endif diff --git a/Include/internal/pycore_stackref.h b/Include/internal/pycore_stackref.h index f23f641a47e25f..d555825fcdc46e 100644 --- a/Include/internal/pycore_stackref.h +++ b/Include/internal/pycore_stackref.h @@ -149,8 +149,7 @@ PyStackRef_FromPyObjectNew(PyObject *obj) { // Make sure we don't take an already tagged value. assert(((uintptr_t)obj & Py_TAG_BITS) == 0); - assert(obj != NULL); - if (_Py_IsImmortal(obj) || _PyObject_HasDeferredRefcount(obj)) { + if (obj == NULL || _Py_IsImmortal(obj) || _PyObject_HasDeferredRefcount(obj)) { return (_PyStackRef){ .bits = (uintptr_t)obj | Py_TAG_DEFERRED }; } else { diff --git a/Objects/dictobject.c b/Objects/dictobject.c index db21961bad266b..166dc137ac263c 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -2324,6 +2324,27 @@ _PyDict_GetItemRef_KnownHash(PyDictObject *op, PyObject *key, Py_hash_t hash, Py return 1; // key is present } +int +_PyDict_GetItem_KnownHash_StackRef(PyDictObject *op, PyObject *key, Py_hash_t hash, _PyStackRef *result) +{ +#ifdef Py_GIL_DISABLED + Py_ssize_t ix = _Py_dict_lookup_threadsafe_stackref(op, key, hash, result); +#else + PyObject *value; + Py_ssize_t ix = _Py_dict_lookup(op, key, hash, &value); + *result = PyStackRef_FromPyObjectSteal(value); +#endif + assert(ix >= 0 || PyStackRef_IsNull(*result)); + if (ix == DKIX_ERROR) { + *result = PyStackRef_NULL; + return -1; + } + if (PyStackRef_IsNull(*result)) { + return 0; // missing key + } + return 1; // key is present +} + int PyDict_GetItemRef(PyObject *op, PyObject *key, PyObject **result) { diff --git a/Objects/object.c b/Objects/object.c index 4b8b6c29266812..f30831fc976a28 100644 --- a/Objects/object.c +++ b/Objects/object.c @@ -1581,6 +1581,106 @@ _PyObject_GetMethod(PyObject *obj, PyObject *name, PyObject **method) return 0; } +int +_PyObject_GetMethodStackRef(PyObject *obj, PyObject *name, _PyStackRef *method) +{ + + int meth_found = 0; + + assert(PyStackRef_IsNull(*method)); + + PyTypeObject *tp = Py_TYPE(obj); + if (!_PyType_IsReady(tp)) { + if (PyType_Ready(tp) < 0) { + return 0; + } + } + + if (tp->tp_getattro != PyObject_GenericGetAttr || !PyUnicode_CheckExact(name)) { + *method = PyStackRef_FromPyObjectSteal(PyObject_GetAttr(obj, name)); + return 0; + } + + // Directly set it to that if a GC cycle happens, the descriptor doesn't get + // evaporated. + // This is why we no longer need a strong reference for this if it's + // deferred. + _PyType_LookupStackRef(tp, name, method); + _PyStackRef descr_st = *method; + descrgetfunc f = NULL; + if (!PyStackRef_IsNull(descr_st)) { + if (_PyType_HasFeature(PyStackRef_TYPE(descr_st), Py_TPFLAGS_METHOD_DESCRIPTOR)) { + meth_found = 1; + } else { + f = PyStackRef_TYPE(descr_st)->tp_descr_get; + if (f != NULL && PyDescr_IsData(PyStackRef_AsPyObjectBorrow(descr_st))) { + *method = PyStackRef_FromPyObjectSteal(f(PyStackRef_AsPyObjectBorrow(descr_st), obj, (PyObject *)Py_TYPE(obj))); + PyStackRef_CLOSE(descr_st); + return 0; + } + } + } + PyObject *dict, *attr; + if ((tp->tp_flags & Py_TPFLAGS_INLINE_VALUES) && + _PyObject_TryGetInstanceAttribute(obj, name, &attr)) { + if (attr != NULL) { + *method = PyStackRef_FromPyObjectSteal(attr); + PyStackRef_XCLOSE(descr_st); + return 0; + } + dict = NULL; + } + else if ((tp->tp_flags & Py_TPFLAGS_MANAGED_DICT)) { + dict = (PyObject *)_PyObject_GetManagedDict(obj); + } + else { + PyObject **dictptr = _PyObject_ComputedDictPointer(obj); + if (dictptr != NULL) { + dict = *dictptr; + } + else { + dict = NULL; + } + } + if (dict != NULL) { + Py_INCREF(dict); + PyObject *item; + if (PyDict_GetItemRef(dict, name, &item) != 0) { + *method = PyStackRef_FromPyObjectSteal(item); + // found or error + Py_DECREF(dict); + PyStackRef_CLOSE(descr_st); + return 0; + } + // not found + Py_DECREF(dict); + } + + if (meth_found) { + *method = descr_st; + return 1; + } + + if (f != NULL) { + *method = PyStackRef_FromPyObjectSteal(f(PyStackRef_AsPyObjectBorrow(descr_st), obj, (PyObject *)Py_TYPE(obj))); + PyStackRef_CLOSE(descr_st); + return 0; + } + + if (!PyStackRef_IsNull(descr_st)) { + *method = descr_st; + return 0; + } + + *method = PyStackRef_NULL; + PyErr_Format(PyExc_AttributeError, + "'%.100s' object has no attribute '%U'", + tp->tp_name, name); + + _PyObject_SetAttributeErrorContext(obj, name); + return 0; +} + /* Generic GetAttr functions - put these in your tp_[gs]etattro slot. */ PyObject * diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 28edd801284b81..d80a42da3e087c 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -5356,6 +5356,58 @@ find_name_in_mro(PyTypeObject *type, PyObject *name, int *error) return res; } + +static void +find_name_in_mro_stackref(PyTypeObject *type, PyObject *name, int *error, _PyStackRef *result) +{ + ASSERT_TYPE_LOCK_HELD(); + + Py_hash_t hash = _PyObject_HashFast(name); + if (hash == -1) { + *error = -1; + *result = PyStackRef_NULL; + return; + } + + /* Look in tp_dict of types in MRO */ + PyObject *mro = lookup_tp_mro(type); + if (mro == NULL) { + if (!is_readying(type)) { + if (PyType_Ready(type) < 0) { + *error = -1; + *result = PyStackRef_NULL; + return; + } + mro = lookup_tp_mro(type); + } + if (mro == NULL) { + *error = 1; + *result = PyStackRef_NULL; + return; + } + } + + /* Keep a strong reference to mro because type->tp_mro can be replaced + during dict lookup, e.g. when comparing to non-string keys. */ + Py_INCREF(mro); + Py_ssize_t n = PyTuple_GET_SIZE(mro); + for (Py_ssize_t i = 0; i < n; i++) { + PyObject *base = PyTuple_GET_ITEM(mro, i); + PyObject *dict = lookup_tp_dict(_PyType_CAST(base)); + assert(dict && PyDict_Check(dict)); + if (_PyDict_GetItem_KnownHash_StackRef((PyDictObject *)dict, name, hash, result) < 0) { + *error = -1; + goto done; + } + if (!PyStackRef_IsNull(*result)) { + break; + } + } + *error = 0; +done: + Py_DECREF(mro); +} + /* Check if the "readied" PyUnicode name is a double-underscore special name. */ static int @@ -5528,6 +5580,95 @@ _PyType_LookupRef(PyTypeObject *type, PyObject *name) return res; } +void +_PyType_LookupStackRef(PyTypeObject *type, PyObject *name, _PyStackRef *result) +{ + int error; + PyInterpreterState *interp = _PyInterpreterState_GET(); + + unsigned int h = MCACHE_HASH_METHOD(type, name); + struct type_cache *cache = get_type_cache(); + struct type_cache_entry *entry = &cache->hashtable[h]; +#ifdef Py_GIL_DISABLED + // synchronize-with other writing threads by doing an acquire load on the sequence + while (1) { + int sequence = _PySeqLock_BeginRead(&entry->sequence); + uint32_t entry_version = _Py_atomic_load_uint32_relaxed(&entry->version); + uint32_t type_version = _Py_atomic_load_uint32_acquire(&type->tp_version_tag); + if (entry_version == type_version && + _Py_atomic_load_ptr_relaxed(&entry->name) == name) { + OBJECT_STAT_INC_COND(type_cache_hits, !is_dunder_name(name)); + OBJECT_STAT_INC_COND(type_cache_dunder_hits, is_dunder_name(name)); + PyObject *value = _Py_atomic_load_ptr_relaxed(&entry->value); + *result = PyStackRef_FromPyObjectNew(value); + // If the sequence is still valid then we're done + if (_PySeqLock_EndRead(&entry->sequence, sequence)) { + return; + } + *result = PyStackRef_NULL; + } + else { + // cache miss + break; + } + } +#else + if (entry->version == type->tp_version_tag && + entry->name == name) { + assert(type->tp_version_tag); + OBJECT_STAT_INC_COND(type_cache_hits, !is_dunder_name(name)); + OBJECT_STAT_INC_COND(type_cache_dunder_hits, is_dunder_name(name)); + Py_XINCREF(entry->value); + return entry->value; + } +#endif + OBJECT_STAT_INC_COND(type_cache_misses, !is_dunder_name(name)); + OBJECT_STAT_INC_COND(type_cache_dunder_misses, is_dunder_name(name)); + + /* We may end up clearing live exceptions below, so make sure it's ours. */ + assert(!PyErr_Occurred()); + + // We need to atomically do the lookup and capture the version before + // anyone else can modify our mro or mutate the type. + + int has_version = 0; + int version = 0; + BEGIN_TYPE_LOCK(); + find_name_in_mro_stackref(type, name, &error, result); + if (MCACHE_CACHEABLE_NAME(name)) { + has_version = assign_version_tag(interp, type); + version = type->tp_version_tag; + } + END_TYPE_LOCK(); + + /* Only put NULL results into cache if there was no error. */ + if (error) { + /* It's not ideal to clear the error condition, + but this function is documented as not setting + an exception, and I don't want to change that. + E.g., when PyType_Ready() can't proceed, it won't + set the "ready" flag, so future attempts to ready + the same type will call it again -- hopefully + in a context that propagates the exception out. + */ + if (error == -1) { + PyErr_Clear(); + } + *result = PyStackRef_NULL; + return; + } + + if (has_version) { +#if Py_GIL_DISABLED + update_cache_gil_disabled(entry, name, version, + PyStackRef_AsPyObjectBorrow(*result)); +#else + PyObject *old_value = update_cache(entry, name, version, PyStackRef_AsPyObjectBorrow(*result)); + Py_DECREF(old_value); +#endif + } +} + PyObject * _PyType_Lookup(PyTypeObject *type, PyObject *name) { diff --git a/Python/bytecodes.c b/Python/bytecodes.c index 846404e28bb18f..4082201a25ea77 100644 --- a/Python/bytecodes.c +++ b/Python/bytecodes.c @@ -1977,19 +1977,17 @@ dummy_func( #endif /* ENABLE_SPECIALIZATION */ } - op(_LOAD_ATTR, (owner -- attr, self_or_null if (oparg & 1))) { + op(_LOAD_ATTR, (owner -- attr: _PyStackRef *, self_or_null if (oparg & 1))) { PyObject *name = GETITEM(FRAME_CO_NAMES, oparg >> 1); - PyObject *attr_o; if (oparg & 1) { /* Designed to work in tandem with CALL, pushes two values. */ - attr_o = NULL; - int is_meth = _PyObject_GetMethod(PyStackRef_AsPyObjectBorrow(owner), name, &attr_o); - if (is_meth) { + *attr = PyStackRef_NULL; + if (_PyObject_GetMethodStackRef(PyStackRef_AsPyObjectBorrow(owner), name, attr)) { /* We can bypass temporary bound method object. meth is unbound method and obj is self. meth | self | arg1 | ... | argN */ - assert(attr_o != NULL); // No errors on this branch + assert(!PyStackRef_IsNull(*attr)); // No errors on this branch self_or_null = owner; // Transfer ownership } else { @@ -2000,17 +1998,16 @@ dummy_func( meth | NULL | arg1 | ... | argN */ DECREF_INPUTS(); - ERROR_IF(attr_o == NULL, error); + ERROR_IF(PyStackRef_IsNull(*attr), error); self_or_null = PyStackRef_NULL; } } else { /* Classic, pushes one value. */ - attr_o = PyObject_GetAttr(PyStackRef_AsPyObjectBorrow(owner), name); + *attr = PyStackRef_FromPyObjectSteal(PyObject_GetAttr(PyStackRef_AsPyObjectBorrow(owner), name)); DECREF_INPUTS(); - ERROR_IF(attr_o == NULL, error); + ERROR_IF(PyStackRef_IsNull(*attr), error); } - attr = PyStackRef_FromPyObjectSteal(attr_o); } macro(LOAD_ATTR) = diff --git a/Python/executor_cases.c.h b/Python/executor_cases.c.h index 93ab068f9de949..2ede2572ab8886 100644 --- a/Python/executor_cases.c.h +++ b/Python/executor_cases.c.h @@ -2200,22 +2200,20 @@ case _LOAD_ATTR: { _PyStackRef owner; - _PyStackRef attr; + _PyStackRef *attr; _PyStackRef self_or_null = PyStackRef_NULL; oparg = CURRENT_OPARG(); owner = stack_pointer[-1]; PyObject *name = GETITEM(FRAME_CO_NAMES, oparg >> 1); - PyObject *attr_o; if (oparg & 1) { /* Designed to work in tandem with CALL, pushes two values. */ - attr_o = NULL; - int is_meth = _PyObject_GetMethod(PyStackRef_AsPyObjectBorrow(owner), name, &attr_o); - if (is_meth) { + *attr = PyStackRef_NULL; + if (_PyObject_GetMethodStackRef(PyStackRef_AsPyObjectBorrow(owner), name, attr)) { /* We can bypass temporary bound method object. meth is unbound method and obj is self. meth | self | arg1 | ... | argN */ - assert(attr_o != NULL); // No errors on this branch + assert(!PyStackRef_IsNull(*attr)); // No errors on this branch self_or_null = owner; // Transfer ownership } else { @@ -2226,18 +2224,17 @@ meth | NULL | arg1 | ... | argN */ PyStackRef_CLOSE(owner); - if (attr_o == NULL) JUMP_TO_ERROR(); + if (PyStackRef_IsNull(*attr)) JUMP_TO_ERROR(); self_or_null = PyStackRef_NULL; } } else { /* Classic, pushes one value. */ - attr_o = PyObject_GetAttr(PyStackRef_AsPyObjectBorrow(owner), name); + *attr = PyStackRef_FromPyObjectSteal(PyObject_GetAttr(PyStackRef_AsPyObjectBorrow(owner), name)); PyStackRef_CLOSE(owner); - if (attr_o == NULL) JUMP_TO_ERROR(); + if (PyStackRef_IsNull(*attr)) JUMP_TO_ERROR(); } - attr = PyStackRef_FromPyObjectSteal(attr_o); - stack_pointer[-1] = attr; + stack_pointer[-1].bits = (uintptr_t)attr; if (oparg & 1) stack_pointer[0] = self_or_null; stack_pointer += (oparg & 1); assert(WITHIN_STACK_BOUNDS()); diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h index 6d902e2c1d9ba8..0c9c93f526c021 100644 --- a/Python/generated_cases.c.h +++ b/Python/generated_cases.c.h @@ -4833,7 +4833,7 @@ _Py_CODEUNIT *this_instr = next_instr - 10; (void)this_instr; _PyStackRef owner; - _PyStackRef attr; + _PyStackRef *attr; _PyStackRef self_or_null = PyStackRef_NULL; // _SPECIALIZE_LOAD_ATTR owner = stack_pointer[-1]; @@ -4855,17 +4855,15 @@ // _LOAD_ATTR { PyObject *name = GETITEM(FRAME_CO_NAMES, oparg >> 1); - PyObject *attr_o; if (oparg & 1) { /* Designed to work in tandem with CALL, pushes two values. */ - attr_o = NULL; - int is_meth = _PyObject_GetMethod(PyStackRef_AsPyObjectBorrow(owner), name, &attr_o); - if (is_meth) { + *attr = PyStackRef_NULL; + if (_PyObject_GetMethodStackRef(PyStackRef_AsPyObjectBorrow(owner), name, attr)) { /* We can bypass temporary bound method object. meth is unbound method and obj is self. meth | self | arg1 | ... | argN */ - assert(attr_o != NULL); // No errors on this branch + assert(!PyStackRef_IsNull(*attr)); // No errors on this branch self_or_null = owner; // Transfer ownership } else { @@ -4876,19 +4874,18 @@ meth | NULL | arg1 | ... | argN */ PyStackRef_CLOSE(owner); - if (attr_o == NULL) goto pop_1_error; + if (PyStackRef_IsNull(*attr)) goto pop_1_error; self_or_null = PyStackRef_NULL; } } else { /* Classic, pushes one value. */ - attr_o = PyObject_GetAttr(PyStackRef_AsPyObjectBorrow(owner), name); + *attr = PyStackRef_FromPyObjectSteal(PyObject_GetAttr(PyStackRef_AsPyObjectBorrow(owner), name)); PyStackRef_CLOSE(owner); - if (attr_o == NULL) goto pop_1_error; + if (PyStackRef_IsNull(*attr)) goto pop_1_error; } - attr = PyStackRef_FromPyObjectSteal(attr_o); } - stack_pointer[-1] = attr; + stack_pointer[-1].bits = (uintptr_t)attr; if (oparg & 1) stack_pointer[0] = self_or_null; stack_pointer += (oparg & 1); assert(WITHIN_STACK_BOUNDS()); From f93588ac12c581bd533d1493610b299d4c171408 Mon Sep 17 00:00:00 2001 From: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com> Date: Sun, 7 Jul 2024 19:47:24 +0800 Subject: [PATCH 2/7] _PyObject_TryGetInstanceAttributeStackRef --- Include/internal/pycore_dict.h | 1 + Include/internal/pycore_object.h | 3 +- Objects/dictobject.c | 91 ++++++++++++++++++++++++++++++++ Objects/object.c | 11 ++-- 4 files changed, 98 insertions(+), 8 deletions(-) diff --git a/Include/internal/pycore_dict.h b/Include/internal/pycore_dict.h index 53bdc71a9bb1d7..25b0e06cfdba78 100644 --- a/Include/internal/pycore_dict.h +++ b/Include/internal/pycore_dict.h @@ -120,6 +120,7 @@ extern int _PyDict_GetItemRef_KnownHash(PyDictObject *op, PyObject *key, Py_hash extern int _PyDict_GetItemRef_Unicode_LockHeld(PyDictObject *op, PyObject *key, PyObject **result); extern int _PyDict_GetItem_KnownHash_StackRef(PyDictObject *op, PyObject *key, Py_hash_t hash, _PyStackRef *result); extern int _PyObjectDict_SetItem(PyTypeObject *tp, PyObject *obj, PyObject **dictptr, PyObject *name, PyObject *value); +extern int _PyDict_GetItemStackRef(PyObject *op, PyObject *key, _PyStackRef *result); extern int _PyDict_Pop_KnownHash( PyDictObject *dict, diff --git a/Include/internal/pycore_object.h b/Include/internal/pycore_object.h index 5f9084bc1b14a7..32350f58643a04 100644 --- a/Include/internal/pycore_object.h +++ b/Include/internal/pycore_object.h @@ -790,7 +790,8 @@ extern int _PyObject_StoreInstanceAttribute(PyObject *obj, PyObject *name, PyObject *value); extern bool _PyObject_TryGetInstanceAttribute(PyObject *obj, PyObject *name, PyObject **attr); - +extern bool _PyObject_TryGetInstanceAttributeStackRef(PyObject *obj, PyObject *name, + _PyStackRef *attr); #ifdef Py_GIL_DISABLED # define MANAGED_DICT_OFFSET (((Py_ssize_t)sizeof(PyObject *))*-1) # define MANAGED_WEAKREF_OFFSET (((Py_ssize_t)sizeof(PyObject *))*-2) diff --git a/Objects/dictobject.c b/Objects/dictobject.c index 166dc137ac263c..f0065ad6d3cafa 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -2363,6 +2363,24 @@ PyDict_GetItemRef(PyObject *op, PyObject *key, PyObject **result) return _PyDict_GetItemRef_KnownHash((PyDictObject *)op, key, hash, result); } +int +_PyDict_GetItemStackRef(PyObject *op, PyObject *key, _PyStackRef *result) +{ + if (!PyDict_Check(op)) { + PyErr_BadInternalCall(); + *result = PyStackRef_NULL; + return -1; + } + + Py_hash_t hash = _PyObject_HashFast(key); + if (hash == -1) { + *result = PyStackRef_NULL; + return -1; + } + + return _PyDict_GetItem_KnownHash_StackRef((PyDictObject *)op, key, hash, result); +} + int _PyDict_GetItemRef_Unicode_LockHeld(PyDictObject *op, PyObject *key, PyObject **result) { @@ -7075,6 +7093,79 @@ _PyObject_TryGetInstanceAttribute(PyObject *obj, PyObject *name, PyObject **attr #endif } +bool +_PyObject_TryGetInstanceAttributeStackRef(PyObject *obj, PyObject *name, _PyStackRef *attr) +{ + assert(PyUnicode_CheckExact(name)); + PyDictValues *values = _PyObject_InlineValues(obj); + if (!FT_ATOMIC_LOAD_UINT8(values->valid)) { + return false; + } + + PyDictKeysObject *keys = CACHED_KEYS(Py_TYPE(obj)); + assert(keys != NULL); + Py_ssize_t ix = _PyDictKeys_StringLookup(keys, name); + if (ix == DKIX_EMPTY) { + *attr = PyStackRef_NULL; + return true; + } + +#ifdef Py_GIL_DISABLED + PyObject *value = _Py_atomic_load_ptr_acquire(&values->values[ix]); + *attr = PyStackRef_FromPyObjectNew(value); + if (value == _Py_atomic_load_ptr_acquire(&values->values[ix])) { + return true; + } + + PyDictObject *dict = _PyObject_GetManagedDict(obj); + if (dict == NULL) { + // No dict, lock the object to prevent one from being + // materialized... + bool success = false; + Py_BEGIN_CRITICAL_SECTION(obj); + + dict = _PyObject_GetManagedDict(obj); + if (dict == NULL) { + // Still no dict, we can read from the values + assert(values->valid); + value = values->values[ix]; + *attr = PyStackRef_FromPyObjectNew(value); + success = true; + } + + Py_END_CRITICAL_SECTION(); + + if (success) { + return true; + } + } + + // We have a dictionary, we'll need to lock it to prevent + // the values from being resized. + assert(dict != NULL); + + bool success; + Py_BEGIN_CRITICAL_SECTION(dict); + + if (dict->ma_values == values && FT_ATOMIC_LOAD_UINT8(values->valid)) { + value = _Py_atomic_load_ptr_relaxed(&values->values[ix]); + *attr = PyStackRef_FromPyObjectNew(value); + success = true; + } else { + // Caller needs to lookup from the dictionary + success = false; + } + + Py_END_CRITICAL_SECTION(); + + return success; +#else + PyObject *value = values->values[ix]; + *attr = PyStackRef_FromPyObjectNew(value); + return true; +#endif +} + int _PyObject_IsInstanceDictEmpty(PyObject *obj) { diff --git a/Objects/object.c b/Objects/object.c index f30831fc976a28..59cbffaa9268ae 100644 --- a/Objects/object.c +++ b/Objects/object.c @@ -1620,11 +1620,10 @@ _PyObject_GetMethodStackRef(PyObject *obj, PyObject *name, _PyStackRef *method) } } } - PyObject *dict, *attr; + PyObject *dict; if ((tp->tp_flags & Py_TPFLAGS_INLINE_VALUES) && - _PyObject_TryGetInstanceAttribute(obj, name, &attr)) { - if (attr != NULL) { - *method = PyStackRef_FromPyObjectSteal(attr); + _PyObject_TryGetInstanceAttributeStackRef(obj, name, method)) { + if (!PyStackRef_IsNull(*method)) { PyStackRef_XCLOSE(descr_st); return 0; } @@ -1644,9 +1643,7 @@ _PyObject_GetMethodStackRef(PyObject *obj, PyObject *name, _PyStackRef *method) } if (dict != NULL) { Py_INCREF(dict); - PyObject *item; - if (PyDict_GetItemRef(dict, name, &item) != 0) { - *method = PyStackRef_FromPyObjectSteal(item); + if (_PyDict_GetItemStackRef(dict, name, method) != 0) { // found or error Py_DECREF(dict); PyStackRef_CLOSE(descr_st); From 1050673ad87a4ef9d87415b823ce25d07763ac64 Mon Sep 17 00:00:00 2001 From: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com> Date: Sun, 15 Sep 2024 18:28:16 +0800 Subject: [PATCH 3/7] cleanup --- Python/bytecodes.c | 5 +++-- Python/executor_cases.c.h | 5 +++-- Python/generated_cases.c.h | 5 +++-- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/Python/bytecodes.c b/Python/bytecodes.c index 4082201a25ea77..6894a0a57455e7 100644 --- a/Python/bytecodes.c +++ b/Python/bytecodes.c @@ -1977,7 +1977,7 @@ dummy_func( #endif /* ENABLE_SPECIALIZATION */ } - op(_LOAD_ATTR, (owner -- attr: _PyStackRef *, self_or_null if (oparg & 1))) { + op(_LOAD_ATTR, (owner -- attr[1], self_or_null if (oparg & 1))) { PyObject *name = GETITEM(FRAME_CO_NAMES, oparg >> 1); if (oparg & 1) { /* Designed to work in tandem with CALL, pushes two values. */ @@ -2004,7 +2004,8 @@ dummy_func( } else { /* Classic, pushes one value. */ - *attr = PyStackRef_FromPyObjectSteal(PyObject_GetAttr(PyStackRef_AsPyObjectBorrow(owner), name)); + PyObject *attr_o = PyObject_GetAttr(PyStackRef_AsPyObjectBorrow(owner), name); + *attr = attr_o == NULL ? PyStackRef_NULL : PyStackRef_FromPyObjectSteal(attr_o); DECREF_INPUTS(); ERROR_IF(PyStackRef_IsNull(*attr), error); } diff --git a/Python/executor_cases.c.h b/Python/executor_cases.c.h index 2ede2572ab8886..8d3bdd96ee86e1 100644 --- a/Python/executor_cases.c.h +++ b/Python/executor_cases.c.h @@ -2204,6 +2204,7 @@ _PyStackRef self_or_null = PyStackRef_NULL; oparg = CURRENT_OPARG(); owner = stack_pointer[-1]; + attr = &stack_pointer[-1]; PyObject *name = GETITEM(FRAME_CO_NAMES, oparg >> 1); if (oparg & 1) { /* Designed to work in tandem with CALL, pushes two values. */ @@ -2230,11 +2231,11 @@ } else { /* Classic, pushes one value. */ - *attr = PyStackRef_FromPyObjectSteal(PyObject_GetAttr(PyStackRef_AsPyObjectBorrow(owner), name)); + PyObject *attr_o = PyObject_GetAttr(PyStackRef_AsPyObjectBorrow(owner), name); + *attr = attr_o == NULL ? PyStackRef_NULL : PyStackRef_FromPyObjectSteal(attr_o); PyStackRef_CLOSE(owner); if (PyStackRef_IsNull(*attr)) JUMP_TO_ERROR(); } - stack_pointer[-1].bits = (uintptr_t)attr; if (oparg & 1) stack_pointer[0] = self_or_null; stack_pointer += (oparg & 1); assert(WITHIN_STACK_BOUNDS()); diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h index 0c9c93f526c021..5ccba597d8af72 100644 --- a/Python/generated_cases.c.h +++ b/Python/generated_cases.c.h @@ -4854,6 +4854,7 @@ /* Skip 8 cache entries */ // _LOAD_ATTR { + attr = &stack_pointer[-1]; PyObject *name = GETITEM(FRAME_CO_NAMES, oparg >> 1); if (oparg & 1) { /* Designed to work in tandem with CALL, pushes two values. */ @@ -4880,12 +4881,12 @@ } else { /* Classic, pushes one value. */ - *attr = PyStackRef_FromPyObjectSteal(PyObject_GetAttr(PyStackRef_AsPyObjectBorrow(owner), name)); + PyObject *attr_o = PyObject_GetAttr(PyStackRef_AsPyObjectBorrow(owner), name); + *attr = attr_o == NULL ? PyStackRef_NULL : PyStackRef_FromPyObjectSteal(attr_o); PyStackRef_CLOSE(owner); if (PyStackRef_IsNull(*attr)) goto pop_1_error; } } - stack_pointer[-1].bits = (uintptr_t)attr; if (oparg & 1) stack_pointer[0] = self_or_null; stack_pointer += (oparg & 1); assert(WITHIN_STACK_BOUNDS()); From f0afe98b06fabaf6504fa437e397ad73e927cfa6 Mon Sep 17 00:00:00 2001 From: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com> Date: Sun, 15 Sep 2024 22:53:26 +0800 Subject: [PATCH 4/7] fix default build --- Objects/object.c | 8 +++++++- Objects/typeobject.c | 3 +-- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/Objects/object.c b/Objects/object.c index 59cbffaa9268ae..9718ba6ddf5274 100644 --- a/Objects/object.c +++ b/Objects/object.c @@ -1584,7 +1584,7 @@ _PyObject_GetMethod(PyObject *obj, PyObject *name, PyObject **method) int _PyObject_GetMethodStackRef(PyObject *obj, PyObject *name, _PyStackRef *method) { - +#ifdef Py_GIL_DISABLED int meth_found = 0; assert(PyStackRef_IsNull(*method)); @@ -1676,6 +1676,12 @@ _PyObject_GetMethodStackRef(PyObject *obj, PyObject *name, _PyStackRef *method) _PyObject_SetAttributeErrorContext(obj, name); return 0; +#else + PyObject *res = NULL; + int err = _PyObject_GetMethod(obj, name, &res); + *method = res == NULL ? PyStackRef_NULL : PyStackRef_FromPyObjectSteal(res); + return err; +#endif } /* Generic GetAttr functions - put these in your tp_[gs]etattro slot. */ diff --git a/Objects/typeobject.c b/Objects/typeobject.c index d80a42da3e087c..0ccbb56d16d654 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -5618,8 +5618,7 @@ _PyType_LookupStackRef(PyTypeObject *type, PyObject *name, _PyStackRef *result) assert(type->tp_version_tag); OBJECT_STAT_INC_COND(type_cache_hits, !is_dunder_name(name)); OBJECT_STAT_INC_COND(type_cache_dunder_hits, is_dunder_name(name)); - Py_XINCREF(entry->value); - return entry->value; + *result = PyStackRef_FromPyObjectNew(entry->value); } #endif OBJECT_STAT_INC_COND(type_cache_misses, !is_dunder_name(name)); From c3fe77742bd01f6fe40cb154f8e40e5bbd913bcd Mon Sep 17 00:00:00 2001 From: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com> Date: Thu, 19 Sep 2024 21:56:41 +0800 Subject: [PATCH 5/7] partially address review --- Include/internal/pycore_stackref.h | 3 ++- Objects/dictobject.c | 27 ++++++++++++++++++--------- Objects/object.c | 27 ++++++++++++++++++++------- Objects/typeobject.c | 7 ++++++- Python/bytecodes.c | 7 ++++++- Python/executor_cases.c.h | 7 ++++++- Python/generated_cases.c.h | 7 ++++++- 7 files changed, 64 insertions(+), 21 deletions(-) diff --git a/Include/internal/pycore_stackref.h b/Include/internal/pycore_stackref.h index d555825fcdc46e..f23f641a47e25f 100644 --- a/Include/internal/pycore_stackref.h +++ b/Include/internal/pycore_stackref.h @@ -149,7 +149,8 @@ PyStackRef_FromPyObjectNew(PyObject *obj) { // Make sure we don't take an already tagged value. assert(((uintptr_t)obj & Py_TAG_BITS) == 0); - if (obj == NULL || _Py_IsImmortal(obj) || _PyObject_HasDeferredRefcount(obj)) { + assert(obj != NULL); + if (_Py_IsImmortal(obj) || _PyObject_HasDeferredRefcount(obj)) { return (_PyStackRef){ .bits = (uintptr_t)obj | Py_TAG_DEFERRED }; } else { diff --git a/Objects/dictobject.c b/Objects/dictobject.c index f0065ad6d3cafa..33cc6d1739d877 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -2327,13 +2327,7 @@ _PyDict_GetItemRef_KnownHash(PyDictObject *op, PyObject *key, Py_hash_t hash, Py int _PyDict_GetItem_KnownHash_StackRef(PyDictObject *op, PyObject *key, Py_hash_t hash, _PyStackRef *result) { -#ifdef Py_GIL_DISABLED Py_ssize_t ix = _Py_dict_lookup_threadsafe_stackref(op, key, hash, result); -#else - PyObject *value; - Py_ssize_t ix = _Py_dict_lookup(op, key, hash, &value); - *result = PyStackRef_FromPyObjectSteal(value); -#endif assert(ix >= 0 || PyStackRef_IsNull(*result)); if (ix == DKIX_ERROR) { *result = PyStackRef_NULL; @@ -7112,7 +7106,12 @@ _PyObject_TryGetInstanceAttributeStackRef(PyObject *obj, PyObject *name, _PyStac #ifdef Py_GIL_DISABLED PyObject *value = _Py_atomic_load_ptr_acquire(&values->values[ix]); - *attr = PyStackRef_FromPyObjectNew(value); + if (value == NULL) { + *attr = PyStackRef_NULL; + } + else { + *attr = PyStackRef_FromPyObjectNew(value); + } if (value == _Py_atomic_load_ptr_acquire(&values->values[ix])) { return true; } @@ -7129,7 +7128,12 @@ _PyObject_TryGetInstanceAttributeStackRef(PyObject *obj, PyObject *name, _PyStac // Still no dict, we can read from the values assert(values->valid); value = values->values[ix]; - *attr = PyStackRef_FromPyObjectNew(value); + if (value != NULL) { + *attr = PyStackRef_FromPyObjectNew(value); + } + else { + *attr = PyStackRef_NULL; + } success = true; } @@ -7161,7 +7165,12 @@ _PyObject_TryGetInstanceAttributeStackRef(PyObject *obj, PyObject *name, _PyStac return success; #else PyObject *value = values->values[ix]; - *attr = PyStackRef_FromPyObjectNew(value); + if (value != NULL) { + *attr = PyStackRef_FromPyObjectNew(value); + } + else { + *attr = PyStackRef_NULL; + } return true; #endif } diff --git a/Objects/object.c b/Objects/object.c index 9718ba6ddf5274..c3bb97a005263e 100644 --- a/Objects/object.c +++ b/Objects/object.c @@ -1601,10 +1601,6 @@ _PyObject_GetMethodStackRef(PyObject *obj, PyObject *name, _PyStackRef *method) return 0; } - // Directly set it to that if a GC cycle happens, the descriptor doesn't get - // evaporated. - // This is why we no longer need a strong reference for this if it's - // deferred. _PyType_LookupStackRef(tp, name, method); _PyStackRef descr_st = *method; descrgetfunc f = NULL; @@ -1614,7 +1610,13 @@ _PyObject_GetMethodStackRef(PyObject *obj, PyObject *name, _PyStackRef *method) } else { f = PyStackRef_TYPE(descr_st)->tp_descr_get; if (f != NULL && PyDescr_IsData(PyStackRef_AsPyObjectBorrow(descr_st))) { - *method = PyStackRef_FromPyObjectSteal(f(PyStackRef_AsPyObjectBorrow(descr_st), obj, (PyObject *)Py_TYPE(obj))); + PyObject *call_res_o = f(PyStackRef_AsPyObjectBorrow(descr_st), obj, (PyObject *)Py_TYPE(obj)); + if (call_res_o != NULL) { + *method = PyStackRef_FromPyObjectSteal(call_res_o); + } + else { + *method = PyStackRef_NULL; + } PyStackRef_CLOSE(descr_st); return 0; } @@ -1659,7 +1661,13 @@ _PyObject_GetMethodStackRef(PyObject *obj, PyObject *name, _PyStackRef *method) } if (f != NULL) { - *method = PyStackRef_FromPyObjectSteal(f(PyStackRef_AsPyObjectBorrow(descr_st), obj, (PyObject *)Py_TYPE(obj))); + PyObject *call_res_o = f(PyStackRef_AsPyObjectBorrow(descr_st), obj, (PyObject *)Py_TYPE(obj)); + if (call_res_o != NULL) { + *method = PyStackRef_FromPyObjectSteal(call_res_o); + } + else { + *method = PyStackRef_NULL; + } PyStackRef_CLOSE(descr_st); return 0; } @@ -1679,7 +1687,12 @@ _PyObject_GetMethodStackRef(PyObject *obj, PyObject *name, _PyStackRef *method) #else PyObject *res = NULL; int err = _PyObject_GetMethod(obj, name, &res); - *method = res == NULL ? PyStackRef_NULL : PyStackRef_FromPyObjectSteal(res); + if (res == NULL) { + *method = PyStackRef_NULL; + } + else { + *method = PyStackRef_FromPyObjectSteal(res); + } return err; #endif } diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 0ccbb56d16d654..30b4980d544c17 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -5600,7 +5600,12 @@ _PyType_LookupStackRef(PyTypeObject *type, PyObject *name, _PyStackRef *result) OBJECT_STAT_INC_COND(type_cache_hits, !is_dunder_name(name)); OBJECT_STAT_INC_COND(type_cache_dunder_hits, is_dunder_name(name)); PyObject *value = _Py_atomic_load_ptr_relaxed(&entry->value); - *result = PyStackRef_FromPyObjectNew(value); + if (value == NULL) { + *result = PyStackRef_NULL; + } + else { + *result = PyStackRef_FromPyObjectNew(value); + } // If the sequence is still valid then we're done if (_PySeqLock_EndRead(&entry->sequence, sequence)) { return; diff --git a/Python/bytecodes.c b/Python/bytecodes.c index 6894a0a57455e7..18bfab5caa30b8 100644 --- a/Python/bytecodes.c +++ b/Python/bytecodes.c @@ -2005,7 +2005,12 @@ dummy_func( else { /* Classic, pushes one value. */ PyObject *attr_o = PyObject_GetAttr(PyStackRef_AsPyObjectBorrow(owner), name); - *attr = attr_o == NULL ? PyStackRef_NULL : PyStackRef_FromPyObjectSteal(attr_o); + if (attr_o == NULL) { + *attr = PyStackRef_NULL; + } + else { + *attr = PyStackRef_FromPyObjectSteal(attr_o); + } DECREF_INPUTS(); ERROR_IF(PyStackRef_IsNull(*attr), error); } diff --git a/Python/executor_cases.c.h b/Python/executor_cases.c.h index 8d3bdd96ee86e1..3757b259ba2e17 100644 --- a/Python/executor_cases.c.h +++ b/Python/executor_cases.c.h @@ -2232,7 +2232,12 @@ else { /* Classic, pushes one value. */ PyObject *attr_o = PyObject_GetAttr(PyStackRef_AsPyObjectBorrow(owner), name); - *attr = attr_o == NULL ? PyStackRef_NULL : PyStackRef_FromPyObjectSteal(attr_o); + if (attr_o == NULL) { + *attr = PyStackRef_NULL; + } + else { + *attr = PyStackRef_FromPyObjectSteal(attr_o); + } PyStackRef_CLOSE(owner); if (PyStackRef_IsNull(*attr)) JUMP_TO_ERROR(); } diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h index 5ccba597d8af72..c2472e1d71bb25 100644 --- a/Python/generated_cases.c.h +++ b/Python/generated_cases.c.h @@ -4882,7 +4882,12 @@ else { /* Classic, pushes one value. */ PyObject *attr_o = PyObject_GetAttr(PyStackRef_AsPyObjectBorrow(owner), name); - *attr = attr_o == NULL ? PyStackRef_NULL : PyStackRef_FromPyObjectSteal(attr_o); + if (attr_o == NULL) { + *attr = PyStackRef_NULL; + } + else { + *attr = PyStackRef_FromPyObjectSteal(attr_o); + } PyStackRef_CLOSE(owner); if (PyStackRef_IsNull(*attr)) goto pop_1_error; } From ba19770f28c210109c9b6113d53e728f0848a888 Mon Sep 17 00:00:00 2001 From: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com> Date: Thu, 14 Nov 2024 19:06:45 +0800 Subject: [PATCH 6/7] Partially address review Co-Authored-By: Sam Gross <655866+colesbury@users.noreply.github.com> --- Include/internal/pycore_dict.h | 2 +- Include/internal/pycore_object.h | 18 ++++++++++++++++++ Objects/dictobject.c | 25 ++++++------------------- Objects/object.c | 8 +++++++- Objects/typeobject.c | 5 +++-- 5 files changed, 35 insertions(+), 23 deletions(-) diff --git a/Include/internal/pycore_dict.h b/Include/internal/pycore_dict.h index c3dc81230cbf01..70bbcb4e15e8d6 100644 --- a/Include/internal/pycore_dict.h +++ b/Include/internal/pycore_dict.h @@ -121,7 +121,7 @@ PyAPI_FUNC(int) _PyDict_SetItem_KnownHash_LockHeld(PyDictObject *mp, PyObject *k PyAPI_FUNC(int) _PyDict_GetItemRef_KnownHash_LockHeld(PyDictObject *op, PyObject *key, Py_hash_t hash, PyObject **result); extern int _PyDict_GetItemRef_KnownHash(PyDictObject *op, PyObject *key, Py_hash_t hash, PyObject **result); extern int _PyDict_GetItemRef_Unicode_LockHeld(PyDictObject *op, PyObject *key, PyObject **result); -extern int _PyDict_GetItem_KnownHash_StackRef(PyDictObject *op, PyObject *key, Py_hash_t hash, _PyStackRef *result); +extern int _PyDict_GetItemStackRef_KnownHash(PyDictObject *op, PyObject *key, Py_hash_t hash, _PyStackRef *result); extern int _PyObjectDict_SetItem(PyTypeObject *tp, PyObject *obj, PyObject **dictptr, PyObject *name, PyObject *value); extern int _PyDict_GetItemStackRef(PyObject *op, PyObject *key, _PyStackRef *result); diff --git a/Include/internal/pycore_object.h b/Include/internal/pycore_object.h index 27d233123920f7..6e44c38b394b01 100644 --- a/Include/internal/pycore_object.h +++ b/Include/internal/pycore_object.h @@ -623,6 +623,24 @@ _Py_TryXGetRef(PyObject **ptr) return NULL; } +// This belongs here rather than pycore_stackref.h because including pycore_object.h +// there causes a circular include. +static inline _PyStackRef +_Py_TryXGetStackRef(PyObject **ptr) +{ + PyObject *value = _Py_atomic_load_ptr(ptr); + if (value == NULL) { + return PyStackRef_NULL; + } + if (_Py_IsImmortal(value) || _PyObject_HasDeferredRefcount(value)) { + return (_PyStackRef){ .bits = (uintptr_t)value | Py_TAG_DEFERRED }; + } + if (_Py_TryIncrefCompare(ptr, value)) { + return PyStackRef_FromPyObjectSteal(value); + } + return PyStackRef_NULL; +} + /* Like Py_NewRef but also optimistically sets _Py_REF_MAYBE_WEAKREF on objects owned by a different thread. */ static inline PyObject * diff --git a/Objects/dictobject.c b/Objects/dictobject.c index ca5a277b419b6d..b6a2ea9ef1acf5 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -2337,7 +2337,7 @@ _PyDict_GetItemRef_KnownHash(PyDictObject *op, PyObject *key, Py_hash_t hash, Py } int -_PyDict_GetItem_KnownHash_StackRef(PyDictObject *op, PyObject *key, Py_hash_t hash, _PyStackRef *result) +_PyDict_GetItemStackRef_KnownHash(PyDictObject *op, PyObject *key, Py_hash_t hash, _PyStackRef *result) { Py_ssize_t ix = _Py_dict_lookup_threadsafe_stackref(op, key, hash, result); assert(ix >= 0 || PyStackRef_IsNull(*result)); @@ -2384,7 +2384,7 @@ _PyDict_GetItemStackRef(PyObject *op, PyObject *key, _PyStackRef *result) return -1; } - return _PyDict_GetItem_KnownHash_StackRef((PyDictObject *)op, key, hash, result); + return _PyDict_GetItemStackRef_KnownHash((PyDictObject *)op, key, hash, result); } int @@ -7148,14 +7148,8 @@ _PyObject_TryGetInstanceAttributeStackRef(PyObject *obj, PyObject *name, _PyStac } #ifdef Py_GIL_DISABLED - PyObject *value = _Py_atomic_load_ptr_acquire(&values->values[ix]); - if (value == NULL) { - *attr = PyStackRef_NULL; - } - else { - *attr = PyStackRef_FromPyObjectNew(value); - } - if (value == _Py_atomic_load_ptr_acquire(&values->values[ix])) { + *attr = _Py_TryXGetStackRef(&values->values[ix]); + if (PyStackRef_AsPyObjectBorrow(*attr) == _Py_atomic_load_ptr_acquire(&values->values[ix])) { return true; } @@ -7170,13 +7164,7 @@ _PyObject_TryGetInstanceAttributeStackRef(PyObject *obj, PyObject *name, _PyStac if (dict == NULL) { // Still no dict, we can read from the values assert(values->valid); - value = values->values[ix]; - if (value != NULL) { - *attr = PyStackRef_FromPyObjectNew(value); - } - else { - *attr = PyStackRef_NULL; - } + *attr = _Py_TryXGetStackRef(&values->values[ix]); success = true; } @@ -7195,8 +7183,7 @@ _PyObject_TryGetInstanceAttributeStackRef(PyObject *obj, PyObject *name, _PyStac Py_BEGIN_CRITICAL_SECTION(dict); if (dict->ma_values == values && FT_ATOMIC_LOAD_UINT8(values->valid)) { - value = _Py_atomic_load_ptr_relaxed(&values->values[ix]); - *attr = PyStackRef_FromPyObjectNew(value); + *attr = _Py_TryXGetStackRef(&values->values[ix]); success = true; } else { // Caller needs to lookup from the dictionary diff --git a/Objects/object.c b/Objects/object.c index ff53e6bc093773..ff8cca26536c31 100644 --- a/Objects/object.c +++ b/Objects/object.c @@ -1636,7 +1636,13 @@ _PyObject_GetMethodStackRef(PyObject *obj, PyObject *name, _PyStackRef *method) } if (tp->tp_getattro != PyObject_GenericGetAttr || !PyUnicode_CheckExact(name)) { - *method = PyStackRef_FromPyObjectSteal(PyObject_GetAttr(obj, name)); + PyObject *attr_o = PyObject_GetAttr(obj, name); + if (attr_o != NULL) { + *method = PyStackRef_FromPyObjectSteal(attr_o); + } + else { + *method = PyStackRef_NULL; + } return 0; } diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 3e7f08b75b5c61..903d881d0756bd 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -5489,11 +5489,12 @@ find_name_in_mro_stackref(PyTypeObject *type, PyObject *name, int *error, _PySta PyObject *base = PyTuple_GET_ITEM(mro, i); PyObject *dict = lookup_tp_dict(_PyType_CAST(base)); assert(dict && PyDict_Check(dict)); - if (_PyDict_GetItem_KnownHash_StackRef((PyDictObject *)dict, name, hash, result) < 0) { + int code = _PyDict_GetItemStackRef_KnownHash((PyDictObject *)dict, name, hash, result); + if (code < 0) { *error = -1; goto done; } - if (!PyStackRef_IsNull(*result)) { + if (code == 1) { break; } } From d38343756e6d95cbd97818b7b29dcea2962fc41a Mon Sep 17 00:00:00 2001 From: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com> Date: Thu, 14 Nov 2024 19:32:28 +0800 Subject: [PATCH 7/7] Write to second slot on stack --- Include/internal/pycore_object.h | 2 +- Include/internal/pycore_opcode_metadata.h | 2 +- Objects/object.c | 3 ++- Python/bytecodes.c | 12 ++++++------ Python/compile.c | 5 +++++ Python/executor_cases.c.h | 16 ++++++++-------- Python/generated_cases.c.h | 16 ++++++++-------- Tools/cases_generator/parsing.py | 16 ++++++++-------- 8 files changed, 39 insertions(+), 33 deletions(-) diff --git a/Include/internal/pycore_object.h b/Include/internal/pycore_object.h index 6e44c38b394b01..3cae612612437e 100644 --- a/Include/internal/pycore_object.h +++ b/Include/internal/pycore_object.h @@ -882,7 +882,7 @@ PyAPI_FUNC(PyObject*) _PyObject_LookupSpecialMethod(PyObject *self, PyObject *at extern int _PyObject_IsAbstract(PyObject *); PyAPI_FUNC(int) _PyObject_GetMethod(PyObject *obj, PyObject *name, PyObject **method); -PyAPI_FUNC(int) _PyObject_GetMethodStackRef(PyObject *obj, PyObject *name, _PyStackRef *method); +PyAPI_FUNC(int) _PyObject_GetMethodStackRef(PyObject *obj, PyObject *name, _PyStackRef *method, _PyStackRef *spare); extern PyObject* _PyObject_NextNotImplemented(PyObject *); // Pickle support. diff --git a/Include/internal/pycore_opcode_metadata.h b/Include/internal/pycore_opcode_metadata.h index 58e583eabbcc46..d510c692b1ee20 100644 --- a/Include/internal/pycore_opcode_metadata.h +++ b/Include/internal/pycore_opcode_metadata.h @@ -739,7 +739,7 @@ int _PyOpcode_num_pushed(int opcode, int oparg) { case LIST_EXTEND: return 1 + (oparg-1); case LOAD_ATTR: - return 1 + (oparg & 1); + return 1 + ((oparg & 1) ? 1 : 0); case LOAD_ATTR_CLASS: return 1 + (oparg & 1); case LOAD_ATTR_CLASS_WITH_METACLASS_CHECK: diff --git a/Objects/object.c b/Objects/object.c index ff8cca26536c31..c11a20049929e4 100644 --- a/Objects/object.c +++ b/Objects/object.c @@ -1621,7 +1621,7 @@ _PyObject_GetMethod(PyObject *obj, PyObject *name, PyObject **method) } int -_PyObject_GetMethodStackRef(PyObject *obj, PyObject *name, _PyStackRef *method) +_PyObject_GetMethodStackRef(PyObject *obj, PyObject *name, _PyStackRef *method, _PyStackRef *spare) { #ifdef Py_GIL_DISABLED int meth_found = 0; @@ -1647,6 +1647,7 @@ _PyObject_GetMethodStackRef(PyObject *obj, PyObject *name, _PyStackRef *method) } _PyType_LookupStackRef(tp, name, method); + *spare = *method; _PyStackRef descr_st = *method; descrgetfunc f = NULL; if (!PyStackRef_IsNull(descr_st)) { diff --git a/Python/bytecodes.c b/Python/bytecodes.c index 373dc1354c7e9d..6a62ba1ddf01d9 100644 --- a/Python/bytecodes.c +++ b/Python/bytecodes.c @@ -2054,19 +2054,19 @@ dummy_func( #endif /* ENABLE_SPECIALIZATION */ } - op(_LOAD_ATTR, (owner -- attr[1], self_or_null if (oparg & 1))) { + op(_LOAD_ATTR, (owner -- attr[1], self_or_null[1] if (oparg & 1))) { PyObject *name = GETITEM(FRAME_CO_NAMES, oparg >> 1); if (oparg & 1) { /* Designed to work in tandem with CALL, pushes two values. */ *attr = PyStackRef_NULL; - int is_meth = _PyObject_GetMethodStackRef(PyStackRef_AsPyObjectBorrow(owner), name, attr); + int is_meth = _PyObject_GetMethodStackRef(PyStackRef_AsPyObjectBorrow(owner), name, attr, self_or_null); if (is_meth) { /* We can bypass temporary bound method object. meth is unbound method and obj is self. meth | self | arg1 | ... | argN */ assert(!PyStackRef_IsNull(*attr)); // No errors on this branch - self_or_null = owner; // Transfer ownership + *self_or_null = owner; // Transfer ownership DEAD(owner); } else { @@ -2078,7 +2078,7 @@ dummy_func( */ DECREF_INPUTS(); ERROR_IF(PyStackRef_IsNull(*attr), error); - self_or_null = PyStackRef_NULL; + *self_or_null = PyStackRef_NULL; } } else { @@ -2090,10 +2090,10 @@ dummy_func( else { *attr = PyStackRef_FromPyObjectSteal(attr_o); } + /* We need to define self_or_null on all paths */ + *self_or_null = PyStackRef_NULL; DECREF_INPUTS(); ERROR_IF(PyStackRef_IsNull(*attr), error); - /* We need to define self_or_null on all paths */ - self_or_null = PyStackRef_NULL; } } diff --git a/Python/compile.c b/Python/compile.c index cbfba7f493e07d..82db6749db207d 100644 --- a/Python/compile.c +++ b/Python/compile.c @@ -1335,6 +1335,11 @@ optimize_and_assemble_code_unit(struct compiler_unit *u, PyObject *const_cache, goto error; } + /* Reserve an extra word on the stack to ensure there is space for uops to + pass at least one item on the stack to a subsequent uop. + */ + stackdepth++; + /** Assembly **/ co = _PyAssemble_MakeCodeObject(&u->u_metadata, const_cache, consts, stackdepth, &optimized_instrs, nlocalsplus, diff --git a/Python/executor_cases.c.h b/Python/executor_cases.c.h index 3637bdaafba208..d8abece25071a4 100644 --- a/Python/executor_cases.c.h +++ b/Python/executor_cases.c.h @@ -2471,16 +2471,17 @@ case _LOAD_ATTR: { _PyStackRef owner; _PyStackRef *attr; - _PyStackRef self_or_null = PyStackRef_NULL; + _PyStackRef *self_or_null = NULL; oparg = CURRENT_OPARG(); owner = stack_pointer[-1]; attr = &stack_pointer[-1]; + self_or_null = &stack_pointer[0]; PyObject *name = GETITEM(FRAME_CO_NAMES, oparg >> 1); if (oparg & 1) { /* Designed to work in tandem with CALL, pushes two values. */ *attr = PyStackRef_NULL; _PyFrame_SetStackPointer(frame, stack_pointer); - int is_meth = _PyObject_GetMethodStackRef(PyStackRef_AsPyObjectBorrow(owner), name, attr); + int is_meth = _PyObject_GetMethodStackRef(PyStackRef_AsPyObjectBorrow(owner), name, attr, self_or_null); stack_pointer = _PyFrame_GetStackPointer(frame); if (is_meth) { /* We can bypass temporary bound method object. @@ -2488,7 +2489,7 @@ meth | self | arg1 | ... | argN */ assert(!PyStackRef_IsNull(*attr)); // No errors on this branch - self_or_null = owner; // Transfer ownership + *self_or_null = owner; // Transfer ownership } else { /* meth is not an unbound method (but a regular attr, or @@ -2499,7 +2500,7 @@ */ PyStackRef_CLOSE(owner); if (PyStackRef_IsNull(*attr)) JUMP_TO_ERROR(); - self_or_null = PyStackRef_NULL; + *self_or_null = PyStackRef_NULL; } } else { @@ -2513,13 +2514,12 @@ else { *attr = PyStackRef_FromPyObjectSteal(attr_o); } + /* We need to define self_or_null on all paths */ + *self_or_null = PyStackRef_NULL; PyStackRef_CLOSE(owner); if (PyStackRef_IsNull(*attr)) JUMP_TO_ERROR(); - /* We need to define self_or_null on all paths */ - self_or_null = PyStackRef_NULL; } - if (oparg & 1) stack_pointer[0] = self_or_null; - stack_pointer += (oparg & 1); + stack_pointer += ((oparg & 1) ? 1 : 0); assert(WITHIN_STACK_BOUNDS()); break; } diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h index bfc4d09d9b6979..a562bb67c9e2da 100644 --- a/Python/generated_cases.c.h +++ b/Python/generated_cases.c.h @@ -5194,7 +5194,7 @@ (void)this_instr; _PyStackRef owner; _PyStackRef *attr; - _PyStackRef self_or_null = PyStackRef_NULL; + _PyStackRef *self_or_null = NULL; // _SPECIALIZE_LOAD_ATTR { owner = stack_pointer[-1]; @@ -5217,12 +5217,13 @@ // _LOAD_ATTR { attr = &stack_pointer[-1]; + self_or_null = &stack_pointer[0]; PyObject *name = GETITEM(FRAME_CO_NAMES, oparg >> 1); if (oparg & 1) { /* Designed to work in tandem with CALL, pushes two values. */ *attr = PyStackRef_NULL; _PyFrame_SetStackPointer(frame, stack_pointer); - int is_meth = _PyObject_GetMethodStackRef(PyStackRef_AsPyObjectBorrow(owner), name, attr); + int is_meth = _PyObject_GetMethodStackRef(PyStackRef_AsPyObjectBorrow(owner), name, attr, self_or_null); stack_pointer = _PyFrame_GetStackPointer(frame); if (is_meth) { /* We can bypass temporary bound method object. @@ -5230,7 +5231,7 @@ meth | self | arg1 | ... | argN */ assert(!PyStackRef_IsNull(*attr)); // No errors on this branch - self_or_null = owner; // Transfer ownership + *self_or_null = owner; // Transfer ownership } else { /* meth is not an unbound method (but a regular attr, or @@ -5241,7 +5242,7 @@ */ PyStackRef_CLOSE(owner); if (PyStackRef_IsNull(*attr)) goto pop_1_error; - self_or_null = PyStackRef_NULL; + *self_or_null = PyStackRef_NULL; } } else { @@ -5255,14 +5256,13 @@ else { *attr = PyStackRef_FromPyObjectSteal(attr_o); } + /* We need to define self_or_null on all paths */ + *self_or_null = PyStackRef_NULL; PyStackRef_CLOSE(owner); if (PyStackRef_IsNull(*attr)) goto pop_1_error; - /* We need to define self_or_null on all paths */ - self_or_null = PyStackRef_NULL; } } - if (oparg & 1) stack_pointer[0] = self_or_null; - stack_pointer += (oparg & 1); + stack_pointer += ((oparg & 1) ? 1 : 0); assert(WITHIN_STACK_BOUNDS()); DISPATCH(); } diff --git a/Tools/cases_generator/parsing.py b/Tools/cases_generator/parsing.py index de31d9b232f9df..c6588b1b962c72 100644 --- a/Tools/cases_generator/parsing.py +++ b/Tools/cases_generator/parsing.py @@ -270,8 +270,8 @@ def cache_effect(self) -> CacheEffect | None: @contextual def stack_effect(self) -> StackEffect | None: - # IDENTIFIER [':' IDENTIFIER [TIMES]] ['if' '(' expression ')'] - # | IDENTIFIER '[' expression ']' + # IDENTIFIER [':' IDENTIFIER [[TIMES]] ['if' '(' expression ')'] + # | IDENTIFIER '[' expression ']' ['if' '(' expression ')'] if tkn := self.expect(lx.IDENTIFIER): type_text = "" if self.expect(lx.COLON): @@ -279,12 +279,6 @@ def stack_effect(self) -> StackEffect | None: if self.expect(lx.TIMES): type_text += " *" cond_text = "" - if self.expect(lx.IF): - self.require(lx.LPAREN) - if not (cond := self.expression()): - raise self.make_syntax_error("Expected condition") - self.require(lx.RPAREN) - cond_text = cond.text.strip() size_text = "" if self.expect(lx.LBRACKET): if type_text or cond_text: @@ -293,6 +287,12 @@ def stack_effect(self) -> StackEffect | None: raise self.make_syntax_error("Expected expression") self.require(lx.RBRACKET) size_text = size.text.strip() + if self.expect(lx.IF): + self.require(lx.LPAREN) + if not (cond := self.expression()): + raise self.make_syntax_error("Expected condition") + self.require(lx.RPAREN) + cond_text = cond.text.strip() return StackEffect(tkn.text, type_text, cond_text, size_text) return None