python
gh-93911: Specialize `LOAD_ATTR` for custom `__getattr__` and `__getattribute__`
Linked to #93911.
For __getattr__, we specialize as per normal as long as the specialized bytecode can succeed without raising AttributeError.
For __getattribute__, a specialized instruction is created to inline the call to __getattribute__.
- Issue: gh-93911
A very significant (15%) increase in hit rates for test_typing. That module represents the best case for this specialisation because nearly all its classes use __getattr__. This is also the highest pyperformance specialization failure apart from property which was fixed by the other PR.
.\PCbuild\amd64\python_d.exe -m test test_typing -R 3:3
On main:
opcode[106].specializable : 1
opcode[106].specialization.success : 3833
opcode[106].specialization.failure : 3648
opcode[106].specialization.hit : 861058
opcode[106].specialization.deferred : 520385
opcode[106].specialization.miss : 91899
opcode[106].specialization.deopt : 1598
opcode[106].execution_count : 119454
opcode[106].specialization.failure_kinds[2] : 1918
opcode[106].specialization.failure_kinds[4] : 214
opcode[106].specialization.failure_kinds[5] : 9
opcode[106].specialization.failure_kinds[9] : 171
opcode[106].specialization.failure_kinds[11] : 251
opcode[106].specialization.failure_kinds[12] : 207
opcode[106].specialization.failure_kinds[14] : 148
opcode[106].specialization.failure_kinds[17] : 201
opcode[106].specialization.failure_kinds[19] : 22
opcode[106].specialization.failure_kinds[21] : 5
opcode[106].specialization.failure_kinds[22] : 14
opcode[106].specialization.failure_kinds[23] : 39
opcode[106].specialization.failure_kinds[25] : 18
opcode[106].specialization.failure_kinds[27] : 464
On this PR:
opcode[106].specializable : 1
opcode[106].specialization.success : 4694
opcode[106].specialization.failure : 2298
opcode[106].specialization.hit : 991871
opcode[106].specialization.deferred : 355737
opcode[106].specialization.miss : 128358
opcode[106].specialization.deopt : 2267
opcode[106].execution_count : 156019
opcode[106].specialization.failure_kinds[2] : 46
opcode[106].specialization.failure_kinds[4] : 214
opcode[106].specialization.failure_kinds[5] : 9
opcode[106].specialization.failure_kinds[9] : 171
opcode[106].specialization.failure_kinds[11] : 251
opcode[106].specialization.failure_kinds[12] : 207
opcode[106].specialization.failure_kinds[14] : 155
opcode[106].specialization.failure_kinds[17] : 201
opcode[106].specialization.failure_kinds[19] : 22
opcode[106].specialization.failure_kinds[21] : 5
opcode[106].specialization.failure_kinds[22] : 13
opcode[106].specialization.failure_kinds[23] : 39
opcode[106].specialization.failure_kinds[25] : 18
opcode[106].specialization.failure_kinds[27] : 464
opcode[106].specialization.failure_kinds[28] : 516
This doesn't seem right to me.
__getattribute__ allows attribute lookup to be overridden.
__getattr__ is for looking attributes after normal lookup has failed.
The existence of __getattr__ shouldn't prevent specialization of normal attribute lookup.
Consider this class
>>> 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: d
We should specialize c.a and c.b; the presence of __getattr__ shouldn't matter.
Interestingly, the current behavior of Python is to check for both __getattribute__ and __getattr__ before calling either of them.
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:
In A.__getattribute__
Raises <class 'AttributeError'>
In A.__getattribute__
In get_b
b
Which is odd, but ideal for specialization, as we don't need to check for __getattr__ at runtime. We can do it at specialization time.
Here's what I think we should do.
- Specialize as normal for classes with a
__getattr__method (in most cases). - Specialize for classes that have a Python
__getattribute__method, and do not have a__getattr__, much likeBINARY_SUBSCR_GETITEMspecializes for classes with a__getitem__method.
Most specializations of LOAD_ATTR deopt if they can't find the attribute. These should work just fine with a __getattr__ method.
Some specializations of LOAD_ATTR, like LOAD_ATTR_PROPERTY can raise an exception and might not be valid if a class has a __getattr__.
We should be able to determine this at specialization time.
Alright. analyze_descriptor will need to be a lot more robust then. It will need to detect cases where it sees a __getattr__ but the attribute is present.
By checking for tp->tp_getattro == slot_tp_getattro and tp->tp_getattro == slot_tp_getattr_hook we can avoid most of the expensive calls to _PyType_Lookup when analyzing the class.
tp->tp_getattro == slot_tp_getattro implies that __getattribute__ is defined but __getattr__ is not. (I think).
tp->tp_getattro == slot_tp_getattr_hook implies that __getattr__ is set.
Therefore we only need to call _PyType_Lookup(tp, &_Py_ID(__getattribute__)) if one of the above is true.
It's alarming that specialisation misses increased more than hits (~75k vs 55k). Maybe it means that for most types with __getattr__, it's expected that there's nothing there? This needs more analysis.
python -m test test_typing -R 3:3
Main 71868a0066a90519ccc6aeb75673ae882aaa03f0:
opcode[106].specializable : 1
opcode[106].specialization.success : 3839
opcode[106].specialization.failure : 3608
opcode[106].specialization.hit : 892801
opcode[106].specialization.deferred : 517029
opcode[106].specialization.miss : 91724
opcode[106].specialization.deopt : 1596
opcode[106].execution_count : 119325
opcode[106].specialization.failure_kinds[2] : 1918
opcode[106].specialization.failure_kinds[4] : 213
opcode[106].specialization.failure_kinds[5] : 9
opcode[106].specialization.failure_kinds[9] : 172
opcode[106].specialization.failure_kinds[11] : 254
opcode[106].specialization.failure_kinds[12] : 165
opcode[106].specialization.failure_kinds[14] : 148
opcode[106].specialization.failure_kinds[17] : 203
opcode[106].specialization.failure_kinds[19] : 20
opcode[106].specialization.failure_kinds[21] : 5
opcode[106].specialization.failure_kinds[22] : 14
opcode[106].specialization.failure_kinds[23] : 39
opcode[106].specialization.failure_kinds[25] : 18
opcode[106].specialization.failure_kinds[27] : 463
This PR 2a931a8faad07375af647f5ed0e8d3548bcb3d37:
opcode[106].specializable : 1
opcode[106].specialization.success : 5321
opcode[106].specialization.failure : 2387
opcode[106].specialization.hit : 944885
opcode[106].specialization.deferred : 391933
opcode[106].specialization.miss : 164726
opcode[106].specialization.deopt : 2937
opcode[106].execution_count : 192307
opcode[106].specialization.failure_kinds[2] : 568
opcode[106].specialization.failure_kinds[4] : 273
opcode[106].specialization.failure_kinds[5] : 9
opcode[106].specialization.failure_kinds[9] : 172
opcode[106].specialization.failure_kinds[11] : 287
opcode[106].specialization.failure_kinds[12] : 165
opcode[106].specialization.failure_kinds[14] : 148
opcode[106].specialization.failure_kinds[17] : 234
opcode[106].specialization.failure_kinds[19] : 20
opcode[106].specialization.failure_kinds[21] : 5
opcode[106].specialization.failure_kinds[22] : 14
opcode[106].specialization.failure_kinds[23] : 39
opcode[106].specialization.failure_kinds[25] : 51
opcode[106].specialization.failure_kinds[27] : 468
The numbers look correct. The total, hit + deferred + miss is unchanged and the number of deferred drops from 34% to 26%, which is a decent improvement. The increase in misses from 6% to 10% may be genuine, due to type instability. You are running this on the tests after all.
Have you tried to gather stats for pyperformance?
When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.
What are we checking for here? ,,, This is what I think we want to do w.r.t overriding these methods:
Is that correct?
Yes.
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).
There's one more special case for custom __getattribute__: where it's set to the defaut lookup (PyObject_GenericGetAttr). In such cases if there's no __getattr__, we can just treat this as a normal attribute lookup. This is why it matters whether it's a custom getattribute or not.
There's one more special case for custom getattribute: where it's set to the defaut lookup (PyObject_GenericGetAttr). In such cases if there's no getattr, we can just treat this as a normal attribute lookup. This is why it matters whether it's a custom getattribute or not.
Do you mean
class C:
__getattribute__ = object.__getattribute__
?
Isn't that the same as not overriding at all?
There's one more special case for custom getattribute: where it's set to the defaut lookup (PyObject_GenericGetAttr). In such cases if there's no getattr, we can just treat this as a normal attribute lookup. This is why it matters whether it's a custom getattribute or not.
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 :).
One minor issue, and we should probably run the buildbots again. Other than that, looks good.
:robot: New build scheduled with the buildbot fleet by @Fidget-Spinner for commit cb7d01edfd5052f92f8fc47656bbf9539febf415 :robot:
If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.
:robot: New build scheduled with the buildbot fleet by @Fidget-Spinner for commit def326ef11321191e577c76adcb5fe3d6f2c82a6 :robot:
If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.
Hey @Fidget-Spinner and @markshannon, I just noticed that LOAD_ATTR_GETATTRIBUTE_OVERRIDDEN, introduced here to support __getattribute__, is never used in the pyperformance benchmark suite. Is that specialization at all important? I know __getattr__ is important -- but __getattribute__ does seem like something rare enough that we might not bother. Does either of you recall why it's needed?
FTR (from a discussion on discord) __getattribute__ was used in _DeprecatedType in the typing module, but not since 3.12.
I think the specialization is worth keeping, but we could move it into the "long tail" discussed in https://github.com/faster-cpython/ideas/issues/447