Skip to content

gh-121459: Deferred LOAD_ATTR (methods) #124101

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

Closed
wants to merge 8 commits into from
Closed

gh-121459: Deferred LOAD_ATTR (methods) #124101

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
be5fa22
Make LOAD_ATTR deferred
Fidget-Spinner Jul 7, 2024
f93588a
_PyObject_TryGetInstanceAttributeStackRef
Fidget-Spinner Jul 7, 2024
1050673
cleanup
Fidget-Spinner Sep 15, 2024
f0afe98
fix default build
Fidget-Spinner Sep 15, 2024
c3fe777
partially address review
Fidget-Spinner Sep 19, 2024
c3448d9
Merge remote-tracking branch 'upstream/main' into deferred_method_final
Fidget-Spinner Nov 14, 2024
ba19770
Partially address review
Fidget-Spinner Nov 14, 2024
d383437
Write to second slot on stack
Fidget-Spinner Nov 14, 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
2 changes: 2 additions & 0 deletions Include/internal/pycore_dict.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,9 @@ 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_GetItemStackRef(PyObject *op, PyObject *key, _PyStackRef *result);

extern int _PyDict_Pop_KnownHash(
PyDictObject *dict,
Expand Down
6 changes: 5 additions & 1 deletion Include/internal/pycore_object.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Member

Choose a reason for hiding this comment

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

Likewise. Anything with "try" in the name needs to be carefully documented.

_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)
Expand Down Expand Up @@ -837,6 +838,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.
Expand Down Expand Up @@ -874,6 +876,8 @@ PyAPI_DATA(int) _Py_SwappedOp[];

extern void _Py_GetConstant_Init(void);

extern void _PyType_LookupStackRef(PyTypeObject *type, PyObject *name, _PyStackRef *result);
Copy link
Member

Choose a reason for hiding this comment

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

Can't this fail? It should probably return an int indicating success/failure/error.


#ifdef __cplusplus
}
#endif
Expand Down
3 changes: 1 addition & 2 deletions Include/internal/pycore_stackref.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
112 changes: 112 additions & 0 deletions Objects/dictobject.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand All @@ -2342,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)
{
Expand Down Expand Up @@ -7054,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)
{
Expand Down
103 changes: 103 additions & 0 deletions Objects/object.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1581,6 +1581,109 @@ _PyObject_GetMethod(PyObject *obj, PyObject *name, PyObject **method)
return 0;
}

int
_PyObject_GetMethodStackRef(PyObject *obj, PyObject *name, _PyStackRef *method)
{
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't have to be in this PR, but we should replace all usages of _PyObject_GetMethod with _PyObject_GetMethodStackRef. Having two similar functions for the same purpose is going to be more difficult to maintain.

#ifdef Py_GIL_DISABLED
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;
Copy link
Contributor

Choose a reason for hiding this comment

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

It's confusing that descr_st and *method refer to the same data, and both are used below. I think it would be more clear if we avoid the aliasing and stick to *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;
if ((tp->tp_flags & Py_TPFLAGS_INLINE_VALUES) &&
_PyObject_TryGetInstanceAttributeStackRef(obj, name, method)) {
if (!PyStackRef_IsNull(*method)) {
PyStackRef_XCLOSE(descr_st);
Comment on lines +1673 to +1675
Copy link
Contributor

Choose a reason for hiding this comment

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

This overwrites *method, but not descr_st, so descr_st may store a stack reference that's not visible to the GC. That seems fragile and potentially a problem.

Copy link
Member Author

Choose a reason for hiding this comment

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

We want to restore *method to descr_st after this in some cases (see below). I guess the safe thing would be to convert it to a strong ref?

Copy link
Contributor

@colesbury colesbury Oct 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think you need both *method and descr_st if you refactor these function.

Alternatively take in a second _PyStackRef * to store the temporary values in a place that is visible to the GC.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the second approach needs to wait for @mpage 's PR on adding an extra stack slot?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh we don't, we can use self_or_null.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok nevermind, this does indeed need Matt's fix. Because we unconditionally write to self_or_null now.

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);
if (_PyDict_GetItemStackRef(dict, name, method) != 0) {
// 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;
#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. */

PyObject *
Expand Down
Loading
Loading