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

Closed

Conversation

Fidget-Spinner
Copy link
Member

@Fidget-Spinner Fidget-Spinner commented Sep 15, 2024

Objects/object.c Outdated Show resolved Hide resolved
Objects/dictobject.c Outdated Show resolved Hide resolved
Include/internal/pycore_stackref.h Outdated Show resolved Hide resolved
Objects/typeobject.c Outdated Show resolved Hide resolved
Python/bytecodes.c Outdated Show resolved Hide resolved
Objects/typeobject.c Show resolved Hide resolved
Include/internal/pycore_dict.h Outdated Show resolved Hide resolved
Objects/dictobject.c Outdated Show resolved Hide resolved
Objects/dictobject.c Outdated Show resolved Hide resolved
Comment on lines +1625 to +1627
_PyObject_TryGetInstanceAttributeStackRef(obj, name, method)) {
if (!PyStackRef_IsNull(*method)) {
PyStackRef_XCLOSE(descr_st);
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.

Objects/object.c Outdated Show resolved Hide resolved
@Fidget-Spinner
Copy link
Member Author

@colesbury gentle ping to respond to the review comments please

Copy link
Member

@markshannon markshannon left a comment

Choose a reason for hiding this comment

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

This seems to add a lot of complexity.
LOAD_ATTR is already the slow path, do you have performance numbers?
Without a significant speedup, this doesn't seem worth it.

For future reference, for large PRs like this, could you make sure there is specific issue where the change can be discussed and justified, rather adding them to a more general (and vague) issue like #121459

// 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.

@@ -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.

@@ -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?

@@ -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.

@@ -2054,20 +2054,19 @@ dummy_func(
#endif /* ENABLE_SPECIALIZATION */
}

op(_LOAD_ATTR, (owner -- attr, self_or_null if (oparg & 1))) {
op(_LOAD_ATTR, (owner -- attr[1], self_or_null[1] if (oparg & 1))) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
op(_LOAD_ATTR, (owner -- attr[1], self_or_null[1] if (oparg & 1))) {
op(_LOAD_ATTR, (owner -- attr, self_or_null if (oparg & 1))) {

You don't need the arrays. The code generator should spill the values to the in-memory stack if necessary.

@@ -1335,6 +1335,11 @@ optimize_and_assemble_code_unit(struct compiler_unit *u, PyObject *const_cache,
goto error;
}

/* Reserve an extra word on the stack to ensure there is space for uops to
Copy link
Member

Choose a reason for hiding this comment

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

Why is this necessary?

@Fidget-Spinner
Copy link
Member Author

@markshannon I'm tempted to wait for Matt to land his LOAD_ATTR specialization PR then take a look again. It may be possible that specialization makes this PR less useful.

@Fidget-Spinner
Copy link
Member Author

I'm closing this because #128164 addresses practically everything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants