Skip to content

gh-93911: Specialize LOAD_ATTR for custom __getattr__ and __getattribute__ #93988

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 21 commits into from
Aug 17, 2022
Merged

gh-93911: Specialize LOAD_ATTR for custom __getattr__ and __getattribute__ #93988

Changes from 1 commit
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
2f483f5
Specialize LOAD_ATTR for custom __getattr__
Fidget-Spinner Jun 18, 2022
de5aab9
fix up comments
Fidget-Spinner Jun 18, 2022
a795602
📜🤖 Added by blurb_it.
blurb-it[bot] Jun 18, 2022
b170204
fix version tag issues
Fidget-Spinner Jun 18, 2022
b4df543
update specialization failure name
Fidget-Spinner Jun 18, 2022
cd81fa9
fix getattr bug
Fidget-Spinner Jun 18, 2022
a2b0121
Fix type size in test_sys
Fidget-Spinner Jun 18, 2022
b0caba5
Merge remote-tracking branch 'upstream/main' into specialize_overridden
Fidget-Spinner Jun 27, 2022
2a931a8
Use Mark's suggestions
Fidget-Spinner Jun 27, 2022
dd89339
fix test_sys
Fidget-Spinner Jun 27, 2022
40e363b
Merge remote-tracking branch 'upstream/main' into specialize_overridden
Fidget-Spinner Jun 30, 2022
db13262
address review
Fidget-Spinner Jun 30, 2022
0185748
Merge remote-tracking branch 'upstream/main' into specialize_overridden
Fidget-Spinner Jul 20, 2022
cab77d8
revert rename
Fidget-Spinner Jul 20, 2022
81adecb
remove unsued failures
Fidget-Spinner Jul 20, 2022
6845656
Address code review
Fidget-Spinner Jul 20, 2022
9412f97
Use Mark's code
Fidget-Spinner Jul 20, 2022
c6f8610
Clean up code a little
Fidget-Spinner Jul 20, 2022
d887293
Merge remote-tracking branch 'upstream/main' into specialize_overridden
Fidget-Spinner Aug 14, 2022
cb7d01e
Update pycore_opcode.h
Fidget-Spinner Aug 14, 2022
def326e
Address review, make things more restrictive
Fidget-Spinner Aug 16, 2022
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
7 changes: 5 additions & 2 deletions Python/specialize.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -389,6 +389,7 @@ miss_counter_start(void) {
#define SPEC_FAIL_ATTR_HAS_MANAGED_DICT 25
#define SPEC_FAIL_ATTR_INSTANCE_ATTRIBUTE 26
#define SPEC_FAIL_ATTR_METACLASS_ATTRIBUTE 27
#define SPEC_FAIL_ATTR_PROPERTY_NOT_PY_FUNCTION 28

/* Binary subscr and store subscr */

Expand Down Expand Up @@ -594,7 +595,9 @@ analyze_descriptor(PyTypeObject *type, PyObject *name, PyObject **descr, int sto
getattribute != interp->callable_cache.object__getattribute__;
has_getattr = _PyType_Lookup(type, &_Py_ID(__getattr__)) != NULL;
if (has_custom_getattribute) {
if (!has_getattr && Py_IS_TYPE(getattribute, &PyFunction_Type)) {
if (getattro_slot == _Py_slot_tp_getattro &&
Copy link
Member

Choose a reason for hiding this comment

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

Why this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

An assertion about which slot is present during specialisation was failing in one of the buildbots. Apparently there are cases where the other slot (which potentially calls __getattr__) still stuck around even with no __getattr__. This may mean the slot had not been called yet (seems strange considering specialised code is supposed to be hot).

Copy link
Member

Choose a reason for hiding this comment

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

Which assertion?
Does that mean the assertion is incorrect, or that this test was?

Copy link
Member Author

Choose a reason for hiding this comment

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

This one https://github.com/python/cpython/pull/93988/files#diff-22e653d878778ca28317261f7826545288d02fd9980cf485d0329f28a34993f2R834.

Copy link
Member

Choose a reason for hiding this comment

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

Strictly, that assert is wrong, in the sense that it's OK if type->tp_getattro == _Py_slot_tp_getattr_hook provided that there it would convert itself to _Py_slot_tp_getattro.

Having said that, it is probably easier to reason about if we assert type->tp_getattro == _Py_slot_tp_getattro, and the impact on performance of the extra test will be negligible.

!has_getattr &&
Py_IS_TYPE(getattribute, &PyFunction_Type)) {
*descr = getattribute;
return GETATTRIBUTE_IS_PYTHON_FUNCTION;
}
Expand Down Expand Up @@ -772,7 +775,7 @@ _Py_Specialize_LoadAttr(PyObject *owner, _Py_CODEUNIT *instr, PyObject *name)
goto fail;
}
if (!Py_IS_TYPE(fget, &PyFunction_Type)) {
SPECIALIZATION_FAIL(LOAD_ATTR, SPEC_FAIL_NOT_PY_FUNCTION);
SPECIALIZATION_FAIL(LOAD_ATTR, SPEC_FAIL_ATTR_PROPERTY_NOT_PY_FUNCTION);
goto fail;
}
uint32_t version = function_check_args(fget, 1, LOAD_ATTR) &&
Expand Down