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-121459: Deferred LOAD_ATTR (methods) #124101

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,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_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);

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

Choose a reason for hiding this comment

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

Please add clear comments as to what this does.
What is it "trying" to do? What happens if it fails? etc.

{
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 *
Expand Down Expand Up @@ -816,7 +834,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 @@ -863,6 +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, _PyStackRef *spare);
Copy link
Member

Choose a reason for hiding this comment

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

Why does this have a "spare" parameter?

extern PyObject* _PyObject_NextNotImplemented(PyObject *);

// Pickle support.
Expand Down Expand Up @@ -900,6 +920,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
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.

108 changes: 108 additions & 0 deletions Objects/dictobject.c
Original file line number Diff line number Diff line change
Expand Up @@ -2336,6 +2336,21 @@ _PyDict_GetItemRef_KnownHash(PyDictObject *op, PyObject *key, Py_hash_t hash, Py
return 1; // key is present
}

int
_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));
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 @@ -2354,6 +2369,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_GetItemStackRef_KnownHash((PyDictObject *)op, key, hash, result);
}

int
_PyDict_GetItemRef_Unicode_LockHeld(PyDictObject *op, PyObject *key, PyObject **result)
{
Expand Down Expand Up @@ -7097,6 +7130,81 @@ _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
*attr = _Py_TryXGetStackRef(&values->values[ix]);
if (PyStackRef_AsPyObjectBorrow(*attr) == _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);
*attr = _Py_TryXGetStackRef(&values->values[ix]);
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)) {
*attr = _Py_TryXGetStackRef(&values->values[ix]);
success = true;
} else {
// Caller needs to lookup from the dictionary
success = false;
}

Py_END_CRITICAL_SECTION();

return success;
#else
PyObject *value = values->values[ix];
if (value != NULL) {
*attr = PyStackRef_FromPyObjectNew(value);
}
else {
*attr = PyStackRef_NULL;
}
return true;
#endif
}

int
_PyObject_IsInstanceDictEmpty(PyObject *obj)
{
Expand Down
123 changes: 123 additions & 0 deletions Objects/object.c
Original file line number Diff line number Diff line change
Expand Up @@ -1620,6 +1620,129 @@ _PyObject_GetMethod(PyObject *obj, PyObject *name, PyObject **method)
return 0;
}

int
_PyObject_GetMethodStackRef(PyObject *obj, PyObject *name, _PyStackRef *method, _PyStackRef *spare)
{
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)) {
PyObject *attr_o = PyObject_GetAttr(obj, name);
if (attr_o != NULL) {
*method = PyStackRef_FromPyObjectSteal(attr_o);
}
else {
*method = PyStackRef_NULL;
}
return 0;
}

_PyType_LookupStackRef(tp, name, method);
*spare = *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))) {
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;
}
}
}
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

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) {
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;
}

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);
if (res == NULL) {
*method = PyStackRef_NULL;
}
else {
*method = PyStackRef_FromPyObjectSteal(res);
}
return err;
#endif
}

/* Generic GetAttr functions - put these in your tp_[gs]etattro slot. */

PyObject *
Expand Down
Loading
Loading