gh-93911: Specialize LOAD_ATTR for custom __getattr__ and __getattribute__
#93988
Conversation
|
A very significant (15%) increase in hit rates for |
Python/ceval.c
Outdated
| if (res) { | ||
| SET_TOP(NULL); | ||
| STACK_GROW((oparg & 1)); | ||
| SET_TOP(res); | ||
| Py_DECREF(owner); | ||
| JUMPBY(INLINE_CACHE_ENTRIES_LOAD_ATTR); | ||
| DISPATCH(); | ||
| } |
There was a problem hiding this comment.
I'm aware that specialized opcodes should have as few branches as possible. But these two opcodes need to respect the getattro semantics.
I could move these out to a function, but the branch would still be there.
|
This doesn't seem right to me. The existence of >>> class C:
... def __init__(self):
... self.a = 1
... b = 2
... def __getattr__(self, name):
... if name == "c":
... return 3
... raise AttributeError(name)
...
>>> c = C()
>>> c.a
1
>>> c.b
2
>>> c.c
3
>>> c.d
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "<stdin>", line 8, in __getattr__
AttributeError: dWe should specialize Interestingly, the current behavior of Python is to check for both def get_b(self, name):
print("In get_b")
if name == "b":
return "b"
else:
raise AttributeError()
class A:
def __getattribute__(self, name):
print("In A.__getattribute__")
if name == "a":
return "a"
else:
type(self).__getattr__ = get_b
raise AttributeError()
try:
print(A().b)
except Exception as e:
print("Raises", type(e), e)
print()
try:
print(A().b)
except Exception as e:
print("Raises", type(e), e)cause the following to be printed: Which is odd, but ideal for specialization, as we don't need to check for |
|
Here's what I think we should do.
Most specializations of |
|
Alright. |
|
By checking for
|
|
It's alarming that specialisation misses increased more than hits (~75k vs 55k). Maybe it means that for most types with |
|
The numbers look correct. The total, Have you tried to gather stats for pyperformance? |
Python/ceval.c
Outdated
| Py_INCREF(f); | ||
| _PyInterpreterFrame *new_frame = _PyFrame_PushUnchecked(tstate, f); | ||
| SET_TOP(NULL); | ||
| int push_null = !(oparg & 1); |
There was a problem hiding this comment.
We push NULL if oparg & 1 (and the line below should be STACK_SHRINK(1-push_null))
There was a problem hiding this comment.
What about:
int shrink_stack = !(oparg & 1);
STACK_SHRINKG(shrink_stack);
The behaviour is equivalent but it does save a subtract operation.
Python/specialize.c
Outdated
| @@ -568,8 +574,47 @@ analyze_descriptor(PyTypeObject *type, PyObject *name, PyObject **descr, int sto | |||
| } | |||
| else { | |||
| if (type->tp_getattro != PyObject_GenericGetAttr) { | |||
| *descr = NULL; | |||
| return GETSET_OVERRIDDEN; | |||
| getattrofunc getattro_slot = type->tp_getattro; | |||
There was a problem hiding this comment.
I wonder if our assumption that getattro_slot == _Py_slot_tp_getattr_hook implies that there is a __getattr__ is true.
It might be that the slot hasn't been updated, even though that seems unlikely.
Could you add an assert that _PyType_Lookup(type, &_Py_ID(__getattr__)) != NULL if has_getattr?
There was a problem hiding this comment.
_Py_slot_tp_getattr_hook handles this case by overwriting itself back to the simpler _Py_slot_tp_getattro. So I think that assert would fail. But in this case would it matter? If there's no __getattr__ isn't that a good thing? It doesn't change the fact that we can just do normal attribute lookup.
There was a problem hiding this comment.
What are we checking for here?
This is what I think we want to do w.r.t overriding these methods:
- Neither
__getattribute__nor__getattr__are overridden, which we already handle. - Both are overridden. Rare, don't specialize.
__getattribute__is overridden,__getattr__is not, and__getattribute__is a Python function, which we add a new specialization for.__getattr__is overridden, two cases:- We expect a value on the instance. We can treat this as a normal instance lookup, as we de-opt if the value is
NULL, falling back to the full lookup. - No attribute on the instance. This is going to be rare, so don't specialize.
- We expect a value on the instance. We can treat this as a normal instance lookup, as we de-opt if the value is
Is that correct?
If so, could you set two booleans has_getattr and has_getattributefirst, then do the case analysis, it would be clearer (to me at least).
|
When you're done making the requested changes, leave the comment: |
Python/specialize.c
Outdated
| @@ -568,8 +574,47 @@ analyze_descriptor(PyTypeObject *type, PyObject *name, PyObject **descr, int sto | |||
| } | |||
| else { | |||
| if (type->tp_getattro != PyObject_GenericGetAttr) { | |||
| *descr = NULL; | |||
| return GETSET_OVERRIDDEN; | |||
| getattrofunc getattro_slot = type->tp_getattro; | |||
There was a problem hiding this comment.
What are we checking for here?
This is what I think we want to do w.r.t overriding these methods:
- Neither
__getattribute__nor__getattr__are overridden, which we already handle. - Both are overridden. Rare, don't specialize.
__getattribute__is overridden,__getattr__is not, and__getattribute__is a Python function, which we add a new specialization for.__getattr__is overridden, two cases:- We expect a value on the instance. We can treat this as a normal instance lookup, as we de-opt if the value is
NULL, falling back to the full lookup. - No attribute on the instance. This is going to be rare, so don't specialize.
- We expect a value on the instance. We can treat this as a normal instance lookup, as we de-opt if the value is
Is that correct?
If so, could you set two booleans has_getattr and has_getattributefirst, then do the case analysis, it would be clearer (to me at least).
Yes.
There's one more special case for custom |
Do you mean class C:
__getattribute__ = object.__getattribute__? Isn't that the same as not overriding at all? |
Yes I was trying to say that it is the same in that case :). |
Co-Authored-By: Mark Shannon <mark@hotpy.org>
|
Ping @markshannon . Is there anything left for me to do? |
|
One minor issue, and we should probably run the buildbots again. |
Fidget-Spinner
added
the
🔨 test-with-buildbots
|
🤖 New build scheduled with the buildbot fleet by @Fidget-Spinner for commit cb7d01e 🤖 If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again. |
bedevere-bot
removed
the
🔨 test-with-buildbots
Fidget-Spinner
added
the
🔨 test-with-buildbots
|
🤖 New build scheduled with the buildbot fleet by @Fidget-Spinner for commit def326e 🤖 If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again. |
bedevere-bot
removed
the
🔨 test-with-buildbots
| @@ -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 && | |||
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Which assertion?
Does that mean the assertion is incorrect, or that this test was?
There was a problem hiding this comment.
There was a problem hiding this comment.
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.
|
Hey @Fidget-Spinner and @markshannon, I just noticed that |
|
FTR (from a discussion on discord) I think the specialization is worth keeping, but we could move it into the "long tail" discussed in faster-cpython/ideas#447 |
Linked to #93911.
For
__getattr__, we specialize as per normal as long as the specialized bytecode can succeed without raisingAttributeError.For
__getattribute__, a specialized instruction is created to inline the call to__getattribute__.