Skip to content
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-115999: Specialize LOAD_ATTR for instance and class receivers in free-threaded builds #128164

Open
wants to merge 57 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 54 commits
Commits
Show all changes
57 commits
Select commit Hold shift + click to select a range
8eeb4fe
Add _PyDictKeys_StringLookupAndVersion
mpage Dec 10, 2024
fcd05a0
Pass shared keys version to specialization
mpage Dec 10, 2024
5c03db0
Add support for enabling each of the instance attribute kinds
mpage Dec 10, 2024
a576748
Only cache deferred descriptors
mpage Dec 10, 2024
8475fd6
Make analyze_descriptor thread-safe
mpage Dec 11, 2024
29c3356
Use an atomic load for GUARD_TYPE_VERSION
mpage Dec 11, 2024
4f9eeb3
Use atomics to load valid bit for inline values in _GUARD_DORV_VALUES…
mpage Dec 11, 2024
e9050fc
Use atomic to load keys version in _GUARD_KEYS_VERSION
mpage Dec 11, 2024
ade57f2
Use an atomic load for managed dict in `_CHECK_ATTR_METHOD_LAZY_DICT`
mpage Dec 11, 2024
0a87264
Enable specialization of method loads
mpage Dec 11, 2024
816e22f
Use atomics for fetching type flags
mpage Dec 11, 2024
ca1e232
Get a strong reference to dict in instance_has_key
mpage Dec 11, 2024
945b61c
Split specialize_dict_access
mpage Dec 11, 2024
feb7d34
Pass type version to specialize_dict_access
mpage Dec 12, 2024
ecfd199
Take a critical section around dict specialization
mpage Dec 12, 2024
9fda5db
Use thread-safe version of _PyDictKeys_StringLookup
mpage Dec 12, 2024
3b2c220
Use atomic load in _CHECK_MANAGED_OBJECT_HAS_VALUES
mpage Dec 12, 2024
408e44b
Make _LOAD_ATTR_INSTANCE_VALUE thread-safe
mpage Dec 12, 2024
16aab70
Make _LOAD_ATTR_WITH_HINT thread-safe
mpage Dec 12, 2024
d0920ea
Specialize instance accesses
mpage Dec 12, 2024
e7cea82
Specialize LOAD_ATTR_SLOT
mpage Dec 12, 2024
8dac8c4
Enable LOAD_ATTR_PROPERTY
mpage Dec 13, 2024
d29b3aa
Checkpoint LOAD_ATTR_GETATTRIBUTE_OVERRIDEN
mpage Dec 13, 2024
b0b8102
Lock dict in instance_has_key
mpage Dec 13, 2024
85dab0d
Lock dict instead of owner when specializing for dict access
mpage Dec 13, 2024
581869e
Use atomic load for valid bit
mpage Dec 13, 2024
96be738
Use _PyDictKeys_StringLookup and lock around it, rather than wasting …
mpage Dec 13, 2024
9afe052
Fix cases_generator bug
mpage Dec 13, 2024
e190a0d
Fix load
mpage Dec 13, 2024
8c78369
Remove FT_UNIMPLEMENTED
mpage Dec 13, 2024
cdf8eb5
Specialize class attribute loads
mpage Dec 13, 2024
1398699
Fix test_type_lookup_mro_reference
mpage Dec 14, 2024
3fbe18b
Remove TYPE_CHANGED
mpage Dec 14, 2024
5aa68cc
Misc clean ups
mpage Dec 14, 2024
e1bf1f4
Check that inline values still valid after taking CS
mpage Dec 14, 2024
396edb1
Enable test_opcache tests
mpage Dec 18, 2024
e5e2508
Skip test that triggers gh-127773 in refleak tests
mpage Dec 20, 2024
fa02260
Merge branch 'main' into gh-115999-load-attr
mpage Dec 20, 2024
47c794f
Add test for LOAD_ATTR_CLASS_WITH_METACLASS_CHECK
mpage Dec 20, 2024
3876bc7
Undo workaround for cases generator bug
mpage Dec 20, 2024
08d14d0
Fix whitespace after merge
mpage Dec 20, 2024
56e9a8a
Double check that dict hasn't changed after locking it
mpage Dec 20, 2024
8ca405b
Use correct type for loading type version
mpage Dec 20, 2024
4c7a4b9
Remove unnecessary comma
mpage Dec 20, 2024
1b787b3
Use atomics when loading oparg
mpage Dec 20, 2024
b868363
Fix formatting
mpage Dec 20, 2024
d6d4c73
Always return type version from analyze_descriptor_load
mpage Dec 20, 2024
9755562
Merge branch 'main' into gh-115999-load-attr-instance-merged
mpage Dec 23, 2024
9673f78
Combine incref/steal into new stackref
mpage Dec 23, 2024
6c3041f
Fix formatting
mpage Dec 23, 2024
a20a4a4
Remove unnecessary error check for PyUnicode_Type.tp_hash
mpage Dec 23, 2024
35b31c6
Check keys kind explicitly
mpage Dec 23, 2024
e5a7ae9
Pass dict from `_CHECK_ATTR_WITH_HINT` to `_LOAD_ATTR_WITH_HINT`
mpage Dec 23, 2024
bb00f5a
Fix unused variable warning
mpage Dec 23, 2024
b6ae487
Update number of specialization failure kinds
mpage Dec 24, 2024
11a351d
Clarify construction of deferred object
mpage Dec 24, 2024
6f8aebf
Add suppression for _PyUnicode_CheckConsistency
mpage Dec 24, 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
10 changes: 10 additions & 0 deletions Include/internal/pycore_dict.h
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,16 @@ extern Py_ssize_t _Py_dict_lookup_threadsafe_stackref(PyDictObject *mp, PyObject

extern Py_ssize_t _PyDict_LookupIndex(PyDictObject *, PyObject *);
extern Py_ssize_t _PyDictKeys_StringLookup(PyDictKeysObject* dictkeys, PyObject *key);

/* Look up a string key in an all unicode dict keys, assign the keys object a version, and
* store it in version.
*
* Returns DKIX_ERROR if key is not a string or if the keys object is not all
* strings.
*
* Returns DKIX_EMPTY if the key is not present.
*/
extern Py_ssize_t _PyDictKeys_StringLookupAndVersion(PyDictKeysObject* dictkeys, PyObject *key, uint32_t *version);
extern Py_ssize_t _PyDictKeys_StringLookupSplit(PyDictKeysObject* dictkeys, PyObject *key);
PyAPI_FUNC(PyObject *)_PyDict_LoadGlobal(PyDictObject *, PyDictObject *, PyObject *);
PyAPI_FUNC(void) _PyDict_LoadGlobalStackRef(PyDictObject *, PyDictObject *, PyObject *, _PyStackRef *);
Expand Down
2 changes: 1 addition & 1 deletion Include/internal/pycore_opcode_metadata.h

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion Include/internal/pycore_uop_metadata.h

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 4 additions & 1 deletion Lib/test/test_capi/test_type.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from test.support import import_helper
from test.support import import_helper, Py_GIL_DISABLED, refleak_helper
import unittest

_testcapi = import_helper.import_module('_testcapi')
Expand Down Expand Up @@ -37,6 +37,9 @@ class D(A, C): pass
# as well
type_freeze(D)

@unittest.skipIf(
Py_GIL_DISABLED and refleak_helper.hunting_for_refleaks(),
"Specialization failure triggers gh-127773")
def test_freeze_meta(self):
"""test PyType_Freeze() with overridden MRO"""
type_freeze = _testcapi.type_freeze
Expand Down
32 changes: 26 additions & 6 deletions Lib/test/test_descr.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import random
import string
import sys
import textwrap
import types
import unittest
import warnings
Expand All @@ -15,6 +16,7 @@
from copy import deepcopy
from contextlib import redirect_stdout
from test import support
from test.support.script_helper import assert_python_ok

try:
import _testcapi
Expand Down Expand Up @@ -5230,6 +5232,7 @@ def test_type_lookup_mro_reference(self):
# Issue #14199: _PyType_Lookup() has to keep a strong reference to
# the type MRO because it may be modified during the lookup, if
# __bases__ is set during the lookup for example.
code = textwrap.dedent("""
class MyKey(object):
def __hash__(self):
return hash('mykey')
Expand All @@ -5245,12 +5248,29 @@ class Base2(object):
mykey = 'from Base2'
mykey2 = 'from Base2'

with self.assertWarnsRegex(RuntimeWarning, 'X'):
X = type('X', (Base,), {MyKey(): 5})
# mykey is read from Base
self.assertEqual(X.mykey, 'from Base')
# mykey2 is read from Base2 because MyKey.__eq__ has set __bases__
self.assertEqual(X.mykey2, 'from Base2')
X = type('X', (Base,), {MyKey(): 5})

bases_before = ",".join([c.__name__ for c in X.__bases__])
print(f"before={bases_before}")

# mykey is initially read from Base, however, the lookup will be perfomed
# again if specialization fails. The second lookup will use the new
# mro set by __eq__.
print(X.mykey)

bases_after = ",".join([c.__name__ for c in X.__bases__])
print(f"after={bases_after}")

# mykey2 is read from Base2 because MyKey.__eq__ has set __bases_
print(f"mykey2={X.mykey2}")
""")
_, out, err = assert_python_ok("-c", code)
err = err.decode()
self.assertRegex(err, "RuntimeWarning: .*X")
out = out.decode()
self.assertRegex(out, "before=Base")
self.assertRegex(out, "after=Base2")
self.assertRegex(out, "mykey2=from Base2")


class PicklingTests(unittest.TestCase):
Expand Down
35 changes: 35 additions & 0 deletions Lib/test/test_generated_cases.py
Original file line number Diff line number Diff line change
Expand Up @@ -1639,6 +1639,41 @@ def test_escaping_call_next_to_cmacro(self):
"""
self.run_cases_test(input, output)

def test_pystackref_frompyobject_new_next_to_cmacro(self):
input = """
inst(OP, (-- out1, out2)) {
PyObject *obj = SPAM();
#ifdef Py_GIL_DISABLED
out1 = PyStackRef_FromPyObjectNew(obj);
#else
out1 = PyStackRef_FromPyObjectNew(obj);
#endif
out2 = PyStackRef_FromPyObjectNew(obj);
}
"""
output = """
TARGET(OP) {
frame->instr_ptr = next_instr;
next_instr += 1;
INSTRUCTION_STATS(OP);
_PyStackRef out1;
_PyStackRef out2;
PyObject *obj = SPAM();
#ifdef Py_GIL_DISABLED
out1 = PyStackRef_FromPyObjectNew(obj);
#else
out1 = PyStackRef_FromPyObjectNew(obj);
#endif
out2 = PyStackRef_FromPyObjectNew(obj);
stack_pointer[0] = out1;
stack_pointer[1] = out2;
stack_pointer += 2;
assert(WITHIN_STACK_BOUNDS());
DISPATCH();
}
"""
self.run_cases_test(input, output)

def test_pop_dead_inputs_all_live(self):
input = """
inst(OP, (a, b --)) {
Expand Down
84 changes: 74 additions & 10 deletions Lib/test/test_opcache.py
Original file line number Diff line number Diff line change
Expand Up @@ -714,11 +714,13 @@ def write(items):
opname = "FOR_ITER_LIST"
self.assert_races_do_not_crash(opname, get_items, read, write)

@requires_specialization
@requires_specialization_ft
def test_load_attr_class(self):
def get_items():
class C:
a = object()
# a must be set to an instance that uses deferred reference
# counting in free-threaded builds
a = type("Foo", (object,), {})
Copy link
Member

@nascheme nascheme Dec 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks a bit mysterious to me. Does this do something different than what class a: [...] does? Calling it an instance (which techically true) is a bit confusing too. I would just say a "set to an object that uses ...". It's a class object. If you need it written like this, I think a little helper function with a docstring explaining why it must be done that way would help:

def make_deferred_ref_count_obj():
    return type("Foo", (object,), {})

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see later there is item.a = type("Foo", (object,), {}) so you want an expression not a statement.


items = []
for _ in range(self.ITEMS):
Expand All @@ -739,12 +741,47 @@ def write(items):
del item.a
except AttributeError:
pass
item.a = object()
item.a = type("Foo", (object,), {})

opname = "LOAD_ATTR_CLASS"
self.assert_races_do_not_crash(opname, get_items, read, write)

@requires_specialization
@requires_specialization_ft
def test_load_attr_class_with_metaclass_check(self):
def get_items():
class Meta(type):
pass

class C(metaclass=Meta):
# a must be set to an instance that uses deferred reference
# counting in free-threaded builds
a = type("Foo", (object,), {})

items = []
for _ in range(self.ITEMS):
item = C
items.append(item)
return items

def read(items):
for item in items:
try:
item.a
except AttributeError:
pass

def write(items):
for item in items:
try:
del item.a
except AttributeError:
pass
item.a = type("Foo", (object,), {})

opname = "LOAD_ATTR_CLASS_WITH_METACLASS_CHECK"
self.assert_races_do_not_crash(opname, get_items, read, write)

@requires_specialization_ft
def test_load_attr_getattribute_overridden(self):
def get_items():
class C:
Expand Down Expand Up @@ -774,7 +811,7 @@ def write(items):
opname = "LOAD_ATTR_GETATTRIBUTE_OVERRIDDEN"
self.assert_races_do_not_crash(opname, get_items, read, write)

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

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

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

@requires_specialization
@requires_specialization_ft
def test_load_attr_method_with_values(self):
def get_items():
class C:
Expand Down Expand Up @@ -914,7 +951,7 @@ def write(items):
opname = "LOAD_ATTR_MODULE"
self.assert_races_do_not_crash(opname, get_items, read, write)

@requires_specialization
@requires_specialization_ft
def test_load_attr_property(self):
def get_items():
class C:
Expand Down Expand Up @@ -944,7 +981,34 @@ def write(items):
opname = "LOAD_ATTR_PROPERTY"
self.assert_races_do_not_crash(opname, get_items, read, write)

@requires_specialization
@requires_specialization_ft
def test_load_attr_slot(self):
def get_items():
class C:
__slots__ = ["a", "b"]

items = []
for i in range(self.ITEMS):
item = C()
item.a = i
item.b = i + self.ITEMS
items.append(item)
return items

def read(items):
for item in items:
item.a
item.b

def write(items):
for item in items:
item.a = 100
item.b = 200

opname = "LOAD_ATTR_SLOT"
self.assert_races_do_not_crash(opname, get_items, read, write)

@requires_specialization_ft
def test_load_attr_with_hint(self):
def get_items():
class C:
Expand Down
41 changes: 32 additions & 9 deletions Objects/dictobject.c
Original file line number Diff line number Diff line change
Expand Up @@ -1129,6 +1129,21 @@ dictkeys_generic_lookup(PyDictObject *mp, PyDictKeysObject* dk, PyObject *key, P
return do_lookup(mp, dk, key, hash, compare_generic);
}

static Py_hash_t
check_keys_and_hash(PyDictKeysObject *dk, PyObject *key)
{
DictKeysKind kind = dk->dk_kind;
if (!PyUnicode_CheckExact(key) || kind == DICT_KEYS_GENERAL) {
return -1;
}
Py_hash_t hash = unicode_get_hash(key);
if (hash == -1) {
hash = PyUnicode_Type.tp_hash(key);
assert(hash != -1);
}
return hash;
}

#ifdef Py_GIL_DISABLED
static Py_ssize_t
unicodekeys_lookup_unicode_threadsafe(PyDictKeysObject* dk, PyObject *key,
Expand Down Expand Up @@ -1167,19 +1182,27 @@ unicodekeys_lookup_split(PyDictKeysObject* dk, PyObject *key, Py_hash_t hash)
Py_ssize_t
_PyDictKeys_StringLookup(PyDictKeysObject* dk, PyObject *key)
{
DictKeysKind kind = dk->dk_kind;
if (!PyUnicode_CheckExact(key) || kind == DICT_KEYS_GENERAL) {
Py_hash_t hash = check_keys_and_hash(dk, key);
if (hash == -1) {
return DKIX_ERROR;
}
Py_hash_t hash = unicode_get_hash(key);
return unicodekeys_lookup_unicode(dk, key, hash);
}

Py_ssize_t
_PyDictKeys_StringLookupAndVersion(PyDictKeysObject *dk, PyObject *key, uint32_t *version)
{
Py_hash_t hash = check_keys_and_hash(dk, key);
if (hash == -1) {
hash = PyUnicode_Type.tp_hash(key);
if (hash == -1) {
PyErr_Clear();
return DKIX_ERROR;
}
return DKIX_ERROR;
}
return unicodekeys_lookup_unicode(dk, key, hash);

Py_ssize_t ix;
LOCK_KEYS(dk);
ix = unicodekeys_lookup_unicode(dk, key, hash);
*version = _PyDictKeys_GetVersionForCurrentState(_PyInterpreterState_GET(), dk);
UNLOCK_KEYS(dk);
return ix;
}

/* Like _PyDictKeys_StringLookup() but only works on split keys. Note
Expand Down
Loading
Loading