-
-
Notifications
You must be signed in to change notification settings - Fork 32.5k
gh-121459: Deferred LOAD_GLOBAL #123128
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
gh-121459: Deferred LOAD_GLOBAL #123128
Changes from 10 commits
803e9f7
b213b6f
11d2fb6
ea8d855
af0c032
1a431de
48bebe9
6e948b4
e23567a
dbbebf9
dbdee10
6d176c0
4ff9f17
c644ebf
64f932a
bfd4400
0c1ee7e
ad33dd1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -10,6 +10,7 @@ extern "C" { | |||||
|
||||||
#include "pycore_object.h" // PyManagedDictPointer | ||||||
#include "pycore_pyatomic_ft_wrappers.h" // FT_ATOMIC_LOAD_SSIZE_ACQUIRE | ||||||
#include "pycore_stackref.h" // _PyStackRef | ||||||
|
||||||
// Unsafe flavor of PyDict_GetItemWithError(): no error checking | ||||||
extern PyObject* _PyDict_GetItemWithError(PyObject *dp, PyObject *key); | ||||||
|
@@ -100,10 +101,11 @@ extern void _PyDictKeys_DecRef(PyDictKeysObject *keys); | |||||
*/ | ||||||
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 _Py_dict_lookup_threadsafe_stackref(PyDictObject *mp, PyObject *key, Py_hash_t hash, _PyStackRef *value_addr); | ||||||
|
||||||
extern Py_ssize_t _PyDict_LookupIndex(PyDictObject *, PyObject *); | ||||||
extern Py_ssize_t _PyDictKeys_StringLookup(PyDictKeysObject* dictkeys, PyObject *key); | ||||||
PyAPI_FUNC(PyObject *)_PyDict_LoadGlobal(PyDictObject *, PyDictObject *, PyObject *); | ||||||
PyAPI_FUNC(void)_PyDict_LoadGlobalStackRef(PyDictObject *, PyDictObject *, PyObject *, _PyStackRef *); | ||||||
Fidget-Spinner marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Likewise |
||||||
|
||||||
/* Consumes references to key and value */ | ||||||
PyAPI_FUNC(int) _PyDict_SetItem_Take2(PyDictObject *op, PyObject *key, PyObject *value); | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1496,6 +1496,97 @@ | |
return ix; | ||
} | ||
|
||
Py_ssize_t | ||
_Py_dict_lookup_threadsafe_stackref(PyDictObject *mp, PyObject *key, Py_hash_t hash, _PyStackRef *value_addr) | ||
{ | ||
PyDictKeysObject *dk; | ||
DictKeysKind kind; | ||
Py_ssize_t ix; | ||
|
||
ensure_shared_on_read(mp); | ||
|
||
dk = _Py_atomic_load_ptr(&mp->ma_keys); | ||
kind = dk->dk_kind; | ||
|
||
if (kind != DICT_KEYS_GENERAL) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is complicated and duplicates logic elsewhere. I think it might be worth only supporting a a single case or two. For example, unicode-only and non-split dict. Fallback to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think it is worth changing anything. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not specializing at all in the free-threaded build so I don't think we want to wait for specialization to be made thread-safe, but we can treat this as a temporary measure and structure the code to reflect that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I changed this case to fall back to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks backwards to me. The common case should be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is still too complicated. We only need to handle the common case of unicode keys. Globals are almost never split dictionaries. I think this should look something like the following. You can refactor it further by extracting a PyDictKeysObject *dk = _Py_atomic_load_ptr(&mp->ma_keys);
if (dk->dk_kind == DICT_KEYS_UNICODE && PyUnicode_CheckExact(key)) {
Py_ssize_t ix = unicodekeys_lookup_unicode_threadsafe(dk, key, hash);
if (ix == DKIX_EMPTY) {
*value_addr = PyStackRef_NULL;
return ix;
}
else if (ix >= 0) {
PyObject **addr_of_value = &DK_UNICODE_ENTRIES(dk)[ix].me_value;
PyObject *value = _Py_atomic_load_ptr(addr_of_value);
if (value == NULL) {
*value_addr = PyStackRef_NULL;
return DKIX_EMPTY;
}
if (_Py_IsImmortal(value) || _PyObject_HasDeferredRefcount(value)) {
*value_addr = (_PyStackRef){ .bits = (uintptr_t)value | Py_TAG_DEFERRED };
return ix;
}
if (_Py_TryIncrefCompare(addr_of_value, value)) {
*value_addr = PyStackRef_FromPyObjectSteal(value);
return ix;
}
}
}
PyObject *obj;
Py_ssize_t ix = _Py_dict_lookup_threadsafe(mp, key, hash, &obj);
if (ix >= 0 && obj != NULL) {
*value_addr = PyStackRef_FromPyObjectSteal(obj);
}
else {
*value_addr = PyStackRef_NULL;
}
return ix; |
||
if (PyUnicode_CheckExact(key)) { | ||
ix = unicodekeys_lookup_unicode_threadsafe(dk, key, hash); | ||
} | ||
else { | ||
ix = unicodekeys_lookup_generic_threadsafe(mp, dk, key, hash); | ||
} | ||
if (ix == DKIX_KEY_CHANGED) { | ||
goto read_failed; | ||
} | ||
|
||
if (ix >= 0) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe put the case |
||
if (kind == DICT_KEYS_SPLIT) { | ||
PyDictValues *values = _Py_atomic_load_ptr(&mp->ma_values); | ||
if (values == NULL) { | ||
goto read_failed; | ||
} | ||
|
||
uint8_t capacity = _Py_atomic_load_uint8_relaxed(&values->capacity); | ||
if (ix >= (Py_ssize_t)capacity) { | ||
goto read_failed; | ||
} | ||
|
||
*value_addr = PyStackRef_FromPyObjectNew(values->values[ix]); | ||
if (PyStackRef_IsNull(*value_addr)) { | ||
goto read_failed; | ||
} | ||
if (values != _Py_atomic_load_ptr(&mp->ma_values)) { | ||
goto read_failed; | ||
} | ||
} | ||
else { | ||
*value_addr = PyStackRef_FromPyObjectNew(DK_UNICODE_ENTRIES(dk)[ix].me_value); | ||
if (PyStackRef_IsNull(*value_addr)) { | ||
goto read_failed; | ||
} | ||
if (dk != _Py_atomic_load_ptr(&mp->ma_keys)) { | ||
goto read_failed; | ||
} | ||
} | ||
} | ||
else { | ||
*value_addr = PyStackRef_NULL; | ||
} | ||
} | ||
else { | ||
ix = dictkeys_generic_lookup_threadsafe(mp, dk, key, hash); | ||
if (ix == DKIX_KEY_CHANGED) { | ||
goto read_failed; | ||
} | ||
if (ix >= 0) { | ||
*value_addr = PyStackRef_FromPyObjectNew(DK_ENTRIES(dk)[ix].me_value); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not quite sure I understand the TSAN failures on dictionaries here. @colesbury do you have a clue? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not thread-safe it the value is not deferred or immortal. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wait but doesn't There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Calling There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ahh okay that makes sense now. So one possible solution would be to introduce another variant that tries to incref and is allowed to fail? Like in https://github.com/python/cpython/blob/main/Objects/dictobject.c#L1435 Or we can check if it's deferred/immortal, and only if it's not we do the TryXGetRef thing, what do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, do the check that it's deferred/immortal and if not do the I think it's best to write it out explicitly first. If there are other uses of it, we can refactor it into a function later. |
||
if (PyStackRef_IsNull(*value_addr)) { | ||
goto read_failed; | ||
} | ||
if (dk != _Py_atomic_load_ptr(&mp->ma_keys)) { | ||
goto read_failed; | ||
} | ||
} | ||
else { | ||
*value_addr = PyStackRef_NULL; | ||
} | ||
} | ||
|
||
return ix; | ||
|
||
PyObject *value; | ||
picnixz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
read_failed: | ||
// In addition to the normal races of the dict being modified the _Py_TryXGetRef | ||
// can all fail if they don't yet have a shared ref count. That can happen here | ||
// or in the *_lookup_* helper. In that case we need to take the lock to avoid | ||
// mutation and do a normal incref which will make them shared. | ||
Py_BEGIN_CRITICAL_SECTION(mp); | ||
ix = _Py_dict_lookup(mp, key, hash, &value); | ||
*value_addr = value == NULL ? PyStackRef_NULL : PyStackRef_FromPyObjectNew(value); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't you use the |
||
Py_END_CRITICAL_SECTION(); | ||
return ix; | ||
} | ||
|
||
#else // Py_GIL_DISABLED | ||
|
||
Py_ssize_t | ||
|
@@ -1506,6 +1597,15 @@ | |
return ix; | ||
} | ||
|
||
Py_ssize_t | ||
_Py_dict_lookup_threadsafe_stackref(PyDictObject *mp, PyObject *key, Py_hash_t hash, _PyStackRef *value_addr) | ||
{ | ||
PyObject *val; | ||
Py_ssize_t ix = _Py_dict_lookup(mp, key, hash, &val); | ||
*value_addr = value == NULL ? PyStackRef_NULL : PyStackRef_FromPyObjectNew(value); | ||
Check failure on line 1605 in Objects/dictobject.c
|
||
return ix; | ||
} | ||
|
||
#endif | ||
|
||
int | ||
|
@@ -2391,31 +2491,31 @@ | |
* 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. | ||
* Stores a new stack reference. | ||
*/ | ||
PyObject * | ||
_PyDict_LoadGlobal(PyDictObject *globals, PyDictObject *builtins, PyObject *key) | ||
void | ||
_PyDict_LoadGlobalStackRef(PyDictObject *globals, PyDictObject *builtins, PyObject *key, _PyStackRef *res) | ||
{ | ||
Py_ssize_t ix; | ||
Py_hash_t hash; | ||
PyObject *value; | ||
|
||
hash = _PyObject_HashFast(key); | ||
if (hash == -1) { | ||
return NULL; | ||
*res = PyStackRef_NULL; | ||
Fidget-Spinner marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
/* namespace 1: globals */ | ||
ix = _Py_dict_lookup_threadsafe(globals, key, hash, &value); | ||
if (ix == DKIX_ERROR) | ||
return NULL; | ||
if (ix != DKIX_EMPTY && value != NULL) | ||
return value; | ||
ix = _Py_dict_lookup_threadsafe_stackref(globals, key, hash, res); | ||
if (ix == DKIX_ERROR) { | ||
*res = PyStackRef_NULL; | ||
} | ||
if (ix != DKIX_EMPTY && !PyStackRef_IsNull(*res)) { | ||
return; | ||
} | ||
|
||
/* namespace 2: builtins */ | ||
ix = _Py_dict_lookup_threadsafe(builtins, key, hash, &value); | ||
assert(ix >= 0 || value == NULL); | ||
return value; | ||
ix = _Py_dict_lookup_threadsafe_stackref(builtins, key, hash, res); | ||
assert(ix >= 0 || PyStackRef_IsNull(*res)); | ||
} | ||
|
||
/* Consumes references to key and value */ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1438,21 +1438,21 @@ dummy_func( | |
|
||
inst(LOAD_FROM_DICT_OR_GLOBALS, (mod_or_class_dict -- v)) { | ||
PyObject *name = GETITEM(FRAME_CO_NAMES, oparg); | ||
PyObject *v_o; | ||
int err = PyMapping_GetOptionalItem(PyStackRef_AsPyObjectBorrow(mod_or_class_dict), name, &v_o); | ||
if (err < 0) { | ||
PyObject *v_o = NULL; | ||
if (PyMapping_GetOptionalItem(PyStackRef_AsPyObjectBorrow(mod_or_class_dict), name, &v_o) < 0) { | ||
ERROR_NO_POP(); | ||
} | ||
if (v_o == NULL) { | ||
if (PyDict_CheckExact(GLOBALS()) | ||
&& PyDict_CheckExact(BUILTINS())) | ||
{ | ||
v_o = _PyDict_LoadGlobal((PyDictObject *)GLOBALS(), | ||
(PyDictObject *)BUILTINS(), | ||
name); | ||
if (v_o == NULL) { | ||
_PyDict_LoadGlobalStackRef((PyDictObject *)GLOBALS(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is inconsistent with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I removed stackrefs from the instruction to leave it as is. |
||
(PyDictObject *)BUILTINS(), | ||
name, | ||
STACK_ENTRY(v)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Practically equivalent to |
||
if (PyStackRef_IsNull(v)) { | ||
if (!_PyErr_Occurred(tstate)) { | ||
/* _PyDict_LoadGlobal() returns NULL without raising | ||
/* _PyDict_LoadGlobalStackRef() sets NULL without raising | ||
* an exception if the key doesn't exist */ | ||
_PyEval_FormatExcCheckArg(tstate, PyExc_NameError, | ||
NAME_ERROR_MSG, name); | ||
|
@@ -1476,8 +1476,10 @@ dummy_func( | |
} | ||
} | ||
} | ||
if (v_o != NULL) { | ||
v = PyStackRef_FromPyObjectSteal(v_o); | ||
} | ||
DECREF_INPUTS(); | ||
v = PyStackRef_FromPyObjectSteal(v_o); | ||
} | ||
|
||
inst(LOAD_NAME, (-- v)) { | ||
|
@@ -1507,10 +1509,9 @@ dummy_func( | |
|
||
op(_LOAD_GLOBAL, ( -- res, null if (oparg & 1))) { | ||
PyObject *name = GETITEM(FRAME_CO_NAMES, oparg>>1); | ||
PyObject *res_o = _PyEval_LoadGlobal(GLOBALS(), BUILTINS(), name); | ||
ERROR_IF(res_o == NULL, error); | ||
_PyEval_LoadGlobalStackRef(GLOBALS(), BUILTINS(), name, STACK_ENTRY(res)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Two suggestions:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will apply 2. but not 1. The reasoning is to keep consistency with _PyEval_LoadGlobal (non-stackref version). |
||
ERROR_IF(PyStackRef_IsNull(res), error); | ||
null = PyStackRef_NULL; | ||
res = PyStackRef_FromPyObjectSteal(res_o); | ||
} | ||
|
||
macro(LOAD_GLOBAL) = | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Uh oh!
There was an error while loading. Please reload this page.