Skip to content

gh-115999: Specialize LOAD_GLOBAL in free-threaded builds #126607

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.

Sign up for GitHub

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 19 commits into from
Nov 21, 2024
Merged

gh-115999: Specialize LOAD_GLOBAL in free-threaded builds #126607

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
05af6b2
Make specialization of LOAD_GLOBAL thread-safe
mpage Sep 25, 2024
5af633c
_Py_GetExecutor needs a CS in free-threaded builds
mpage Oct 28, 2024
7b3df6f
Undo workaround for cases generator bug
mpage Nov 8, 2024
8f6239d
Fix unused function warning
mpage Nov 8, 2024
49ec70a
Double check that keys are still valid
mpage Nov 8, 2024
0e50451
Fix formatting
mpage Nov 8, 2024
72519ca
Fix linkage
mpage Nov 9, 2024
d55397b
`_Py_TryIncrefCompare` can't escape
mpage Nov 9, 2024
ad9a9d0
Use atomics for setting keys version for split keys
mpage Nov 16, 2024
5b7e65c
Ensure dict is shared when assigning keys version
mpage Nov 16, 2024
129300b
Don't check keys after loading them
mpage Nov 17, 2024
3440429
Support deferred refcounting
mpage Nov 17, 2024
01041a4
Merge branch 'main' into gh-115999-tlbc-load-global
mpage Nov 18, 2024
bfe1c93
Fix param name
mpage Nov 19, 2024
d5258e0
Always mark keys shared
mpage Nov 19, 2024
852be38
Update comment
mpage Nov 19, 2024
e11e0ae
Merge branch 'main' into gh-115999-tlbc-load-global
mpage Nov 20, 2024
01f4143
Don't pass reason to unspecialize
mpage Nov 20, 2024
b922dfe
Merge branch 'main' into gh-115999-tlbc-load-global
mpage Nov 21, 2024
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
19 changes: 18 additions & 1 deletion Lib/test/test_opcache.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -546,7 +546,6 @@ def count_args(self, *args):


@threading_helper.requires_working_threading()
@requires_specialization
class TestRacesDoNotCrash(TestBase):
# Careful with these. Bigger numbers have a higher chance of catching bugs,
# but you can also burn through a *ton* of type/dict/function versions:
Expand Down Expand Up @@ -588,6 +587,7 @@ def assert_races_do_not_crash(
for writer in writers:
writer.join()

@requires_specialization
def test_binary_subscr_getitem(self):
def get_items():
class C:
Expand Down Expand Up @@ -617,6 +617,7 @@ def write(items):
opname = "BINARY_SUBSCR_GETITEM"
self.assert_races_do_not_crash(opname, get_items, read, write)

@requires_specialization
def test_binary_subscr_list_int(self):
def get_items():
items = []
Expand All @@ -640,6 +641,7 @@ def write(items):
opname = "BINARY_SUBSCR_LIST_INT"
self.assert_races_do_not_crash(opname, get_items, read, write)

@requires_specialization
def test_for_iter_gen(self):
def get_items():
def g():
Expand Down Expand Up @@ -671,6 +673,7 @@ def write(items):
opname = "FOR_ITER_GEN"
self.assert_races_do_not_crash(opname, get_items, read, write)

@requires_specialization
def test_for_iter_list(self):
def get_items():
items = []
Expand All @@ -692,6 +695,7 @@ def write(items):
opname = "FOR_ITER_LIST"
self.assert_races_do_not_crash(opname, get_items, read, write)

@requires_specialization
def test_load_attr_class(self):
def get_items():
class C:
Expand Down Expand Up @@ -721,6 +725,7 @@ def write(items):
opname = "LOAD_ATTR_CLASS"
self.assert_races_do_not_crash(opname, get_items, read, write)

@requires_specialization
def test_load_attr_getattribute_overridden(self):
def get_items():
class C:
Expand Down Expand Up @@ -750,6 +755,7 @@ def write(items):
opname = "LOAD_ATTR_GETATTRIBUTE_OVERRIDDEN"
self.assert_races_do_not_crash(opname, get_items, read, write)

@requires_specialization
def test_load_attr_instance_value(self):
def get_items():
class C:
Expand All @@ -773,6 +779,7 @@ def write(items):
opname = "LOAD_ATTR_INSTANCE_VALUE"
self.assert_races_do_not_crash(opname, get_items, read, write)

@requires_specialization
def test_load_attr_method_lazy_dict(self):
def get_items():
class C(Exception):
Expand Down Expand Up @@ -802,6 +809,7 @@ def write(items):
opname = "LOAD_ATTR_METHOD_LAZY_DICT"
self.assert_races_do_not_crash(opname, get_items, read, write)

@requires_specialization
def test_load_attr_method_no_dict(self):
def get_items():
class C:
Expand Down Expand Up @@ -832,6 +840,7 @@ def write(items):
opname = "LOAD_ATTR_METHOD_NO_DICT"
self.assert_races_do_not_crash(opname, get_items, read, write)

@requires_specialization
def test_load_attr_method_with_values(self):
def get_items():
class C:
Expand Down Expand Up @@ -861,6 +870,7 @@ def write(items):
opname = "LOAD_ATTR_METHOD_WITH_VALUES"
self.assert_races_do_not_crash(opname, get_items, read, write)

@requires_specialization
def test_load_attr_module(self):
def get_items():
items = []
Expand All @@ -885,6 +895,7 @@ def write(items):
opname = "LOAD_ATTR_MODULE"
self.assert_races_do_not_crash(opname, get_items, read, write)

@requires_specialization
def test_load_attr_property(self):
def get_items():
class C:
Expand Down Expand Up @@ -914,6 +925,7 @@ def write(items):
opname = "LOAD_ATTR_PROPERTY"
self.assert_races_do_not_crash(opname, get_items, read, write)

@requires_specialization
def test_load_attr_with_hint(self):
def get_items():
class C:
Expand All @@ -940,6 +952,7 @@ def write(items):
opname = "LOAD_ATTR_WITH_HINT"
self.assert_races_do_not_crash(opname, get_items, read, write)

@requires_specialization_ft
def test_load_global_module(self):
def get_items():
items = []
Expand All @@ -961,6 +974,7 @@ def write(items):
opname, get_items, read, write, check_items=True
)

@requires_specialization
def test_store_attr_instance_value(self):
def get_items():
class C:
Expand All @@ -983,6 +997,7 @@ def write(items):
opname = "STORE_ATTR_INSTANCE_VALUE"
self.assert_races_do_not_crash(opname, get_items, read, write)

@requires_specialization
def test_store_attr_with_hint(self):
def get_items():
class C:
Expand All @@ -1008,6 +1023,7 @@ def write(items):
opname = "STORE_ATTR_WITH_HINT"
self.assert_races_do_not_crash(opname, get_items, read, write)

@requires_specialization
def test_store_subscr_list_int(self):
def get_items():
items = []
Expand All @@ -1031,6 +1047,7 @@ def write(items):
opname = "STORE_SUBSCR_LIST_INT"
self.assert_races_do_not_crash(opname, get_items, read, write)

@requires_specialization
def test_unpack_sequence_list(self):
def get_items():
items = []
Expand Down
27 changes: 21 additions & 6 deletions Objects/dictobject.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1701,7 +1701,7 @@ insert_combined_dict(PyInterpreterState *interp, PyDictObject *mp,
}

_PyDict_NotifyEvent(interp, PyDict_EVENT_ADDED, mp, key, value);
mp->ma_keys->dk_version = 0;
FT_ATOMIC_STORE_UINT32_RELAXED(mp->ma_keys->dk_version, 0);

Py_ssize_t hashpos = find_empty_slot(mp->ma_keys, hash);
dictkeys_set_index(mp->ma_keys, hashpos, mp->ma_keys->dk_nentries);
Expand Down Expand Up @@ -2679,7 +2679,7 @@ delitem_common(PyDictObject *mp, Py_hash_t hash, Py_ssize_t ix,
ASSERT_CONSISTENT(mp);
}
else {
mp->ma_keys->dk_version = 0;
FT_ATOMIC_STORE_UINT32_RELAXED(mp->ma_keys->dk_version, 0);
dictkeys_set_index(mp->ma_keys, hashpos, DKIX_DUMMY);
if (DK_IS_UNICODE(mp->ma_keys)) {
PyDictUnicodeEntry *ep = &DK_UNICODE_ENTRIES(mp->ma_keys)[ix];
Expand Down Expand Up @@ -4498,7 +4498,7 @@ dict_popitem_impl(PyDictObject *self)
return NULL;
}
}
self->ma_keys->dk_version = 0;
FT_ATOMIC_STORE_UINT32_RELAXED(self->ma_keys->dk_version, 0);

/* Pop last item */
PyObject *key, *value;
Expand Down Expand Up @@ -7390,17 +7390,32 @@ _PyDictKeys_DecRef(PyDictKeysObject *keys)
dictkeys_decref(interp, keys, false);
}

// In free-threaded builds the caller must ensure that the keys object is not
// being mutated concurrently by another thread.
uint32_t _PyDictKeys_GetVersionForCurrentState(PyInterpreterState *interp,
PyDictKeysObject *dictkeys)
{
if (dictkeys->dk_version != 0) {
return dictkeys->dk_version;
uint32_t dk_version = FT_ATOMIC_LOAD_UINT32_RELAXED(dictkeys->dk_version);
if (dk_version != 0) {
return dk_version;
}
#ifdef Py_GIL_DISABLED
uint32_t v;
do {
v = _Py_atomic_load_uint32_relaxed(
&interp->dict_state.next_keys_version);
if (v == 0) {
return 0;
}
} while (!_Py_atomic_compare_exchange_uint32(
&interp->dict_state.next_keys_version, &v, v + 1));
#else
if (interp->dict_state.next_keys_version == 0) {
return 0;
}
uint32_t v = interp->dict_state.next_keys_version++;
dictkeys->dk_version = v;
#endif
FT_ATOMIC_STORE_UINT32_RELAXED(dictkeys->dk_version, v);
return v;
}

Expand Down
2 changes: 2 additions & 0 deletions Objects/funcobject.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,12 +289,14 @@ functions is running.

*/

#ifndef Py_GIL_DISABLED
static inline struct _func_version_cache_item *
get_cache_item(PyInterpreterState *interp, uint32_t version)
{
return interp->func_state.func_version_cache +
(version % FUNC_VERSION_CACHE_SIZE);
}
#endif

void
_PyFunction_SetVersion(PyFunctionObject *func, uint32_t version)
Expand Down
43 changes: 33 additions & 10 deletions Python/bytecodes.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1569,7 +1569,7 @@ dummy_func(
};

specializing op(_SPECIALIZE_LOAD_GLOBAL, (counter/1 -- )) {
#if ENABLE_SPECIALIZATION
#if ENABLE_SPECIALIZATION_FT
if (ADAPTIVE_COUNTER_TRIGGERS(counter)) {
PyObject *name = GETITEM(FRAME_CO_NAMES, oparg>>1);
next_instr = this_instr;
Expand All @@ -1578,7 +1578,7 @@ dummy_func(
}
OPCODE_DEFERRED_INC(LOAD_GLOBAL);
ADVANCE_ADAPTIVE_COUNTER(this_instr[1].counter);
#endif /* ENABLE_SPECIALIZATION */
#endif /* ENABLE_SPECIALIZATION_FT */
}

// res[1] because we need a pointer to res to pass it to _PyEval_LoadGlobalStackRef
Expand All @@ -1599,47 +1599,70 @@ dummy_func(
op(_GUARD_GLOBALS_VERSION, (version/1 --)) {
PyDictObject *dict = (PyDictObject *)GLOBALS();
DEOPT_IF(!PyDict_CheckExact(dict));
DEOPT_IF(dict->ma_keys->dk_version != version);
assert(DK_IS_UNICODE(dict->ma_keys));
PyDictKeysObject *keys = FT_ATOMIC_LOAD_PTR_ACQUIRE(dict->ma_keys);
DEOPT_IF(FT_ATOMIC_LOAD_UINT32_RELAXED(keys->dk_version) != version);
assert(DK_IS_UNICODE(keys));
}

op(_GUARD_GLOBALS_VERSION_PUSH_KEYS, (version / 1 -- globals_keys: PyDictKeysObject *))
{
PyDictObject *dict = (PyDictObject *)GLOBALS();
DEOPT_IF(!PyDict_CheckExact(dict));
DEOPT_IF(dict->ma_keys->dk_version != version);
globals_keys = dict->ma_keys;
PyDictKeysObject *keys = FT_ATOMIC_LOAD_PTR_ACQUIRE(dict->ma_keys);
DEOPT_IF(FT_ATOMIC_LOAD_UINT32_RELAXED(keys->dk_version) != version);
globals_keys = keys;
assert(DK_IS_UNICODE(globals_keys));
}

op(_GUARD_BUILTINS_VERSION_PUSH_KEYS, (version / 1 -- builtins_keys: PyDictKeysObject *))
{
PyDictObject *dict = (PyDictObject *)BUILTINS();
DEOPT_IF(!PyDict_CheckExact(dict));
DEOPT_IF(dict->ma_keys->dk_version != version);
builtins_keys = dict->ma_keys;
PyDictKeysObject *keys = FT_ATOMIC_LOAD_PTR_ACQUIRE(dict->ma_keys);
DEOPT_IF(FT_ATOMIC_LOAD_UINT32_RELAXED(keys->dk_version) != version);
builtins_keys = keys;
assert(DK_IS_UNICODE(builtins_keys));
}

op(_LOAD_GLOBAL_MODULE_FROM_KEYS, (index/1, globals_keys: PyDictKeysObject* -- res, null if (oparg & 1))) {
PyDictUnicodeEntry *entries = DK_UNICODE_ENTRIES(globals_keys);
PyObject *res_o = entries[index].me_value;
PyObject *res_o = FT_ATOMIC_LOAD_PTR_RELAXED(entries[index].me_value);
DEAD(globals_keys);
SYNC_SP();
DEOPT_IF(res_o == NULL);
#if Py_GIL_DISABLED
int increfed = _Py_TryIncrefCompare(&entries[index].me_value, res_o);
DEOPT_IF(!increfed);
PyDictObject *dict = (PyDictObject*) GLOBALS();
if (globals_keys != _Py_atomic_load_ptr_acquire(&dict->ma_keys)) {
Py_DECREF(res_o);
DEOPT_IF(true);
}
#else
Py_INCREF(res_o);
#endif
STAT_INC(LOAD_GLOBAL, hit);
null = PyStackRef_NULL;
res = PyStackRef_FromPyObjectSteal(res_o);
}

op(_LOAD_GLOBAL_BUILTINS_FROM_KEYS, (index/1, builtins_keys: PyDictKeysObject* -- res, null if (oparg & 1))) {
PyDictUnicodeEntry *entries = DK_UNICODE_ENTRIES(builtins_keys);
PyObject *res_o = entries[index].me_value;
PyObject *res_o = FT_ATOMIC_LOAD_PTR_RELAXED(entries[index].me_value);
DEAD(builtins_keys);
SYNC_SP();
DEOPT_IF(res_o == NULL);
#if Py_GIL_DISABLED
int increfed = _Py_TryIncrefCompare(&entries[index].me_value, res_o);
DEOPT_IF(!increfed);
PyDictObject *dict = (PyDictObject*) BUILTINS();
if (builtins_keys != _Py_atomic_load_ptr_acquire(&dict->ma_keys)) {
Py_DECREF(res_o);
DEOPT_IF(true);
}
#else
Py_INCREF(res_o);
#endif
STAT_INC(LOAD_GLOBAL, hit);
null = PyStackRef_NULL;
res = PyStackRef_FromPyObjectSteal(res_o);
Expand Down
Loading
Loading