GH-94822: Respect metaclasses when caching class attributes #94863
Conversation
|
Note to self: this might be able to be automatically backported by the bots. We just need to change all instances of the term |
| if (metaclass->tp_getattro != PyType_Type.tp_getattro) { | ||
| // The metaclass defines __getattribute__. All bets are off: | ||
| SPECIALIZATION_FAIL(LOAD_ATTR, SPEC_FAIL_OVERRIDDEN); | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
Do you think we should just play safe and fail whenever we see a non-type type? (ie whenever there's a metaclass)
There was a problem hiding this comment.
We have simple ways of checking for all known problematic cases (names present in the metaclass, __getattribute__ defined, mutating metaclasses). I guess we could do that, but it sort of feels like overkill to me.
I'm currently running the benchmarks and gathering stats for this branch, but I worry that disallowing all metaclasses would really hurt benchmarks like sympy which have tons of metaclasses. So I'd want to make sure we don't suffer any big regressions before disallowing them entirely.
There was a problem hiding this comment.
Thanks for the quick work @brandtbucher
I don't know if this helps but if you want to time SymPy specifically then one way is:
import sympy
sympy.test()
That takes about half an hour to run on this computer.
Comparing different CPython versions in CI shows that 3.11.0b4 currently gives a speedup for some jobs and is neutral for others:
https://github.com/sympy/sympy/actions/runs/2662492934
If you look at for example the test2 matrix then the timings from there are:
3.8: 19m28s
3.9: 16m44s
3.10: 16m4s
3.11.0b4: 14m45s
The test2 job does roughly half of what sympy.test() does and can be reproduce directly with sympy.test(split='2/2'). The 3.10 timing is shown in CI as test2-latest separately from the test2 matrix of different Python versions.
There was a problem hiding this comment.
Thanks, I'll take a look.
Side note, I just realized that potentially removing support for any metaclasses here would probably (further) trash enum performance. :( So that's also something we want visibility into before going that route.
|
Also could you run |
|
Looks like this patch makes things about 1% slower. It's not immediately clear to me if this is mainly from removing illegitimate specializations, the increased cost of specializing class attribute loads, or the overhead of additional metaclass version checks: Total hit rates for
|
| Name | Count | Self | Cumulative | Miss ratio |
|---|---|---|---|---|
| LOAD_ATTR_CLASS | 102,062,535 | 0.2% | 96.5% | 1.6% |
specialization stats for LOAD_ATTR family
| Kind | Count | Ratio |
|---|---|---|
| unquickened | 193618648 | 3.4% |
| specialization.deferred | 629431714 | 11.2% |
| specialization.deopt | 2359119 | 0.0% |
| hit | 4675648068 | 83.1% |
| miss | 129400942 | 2.3% |
Specialization attempts
| Count | Ratio | |
|---|---|---|
| Success | 4,958,258 | 64.8% |
| Failure | 2,693,208 | 35.2% |
| Failure kind | Count | Ratio |
|---|---|---|
| not managed dict | 586,470 | 21.8% |
| overridden | 540,857 | 20.1% |
| method | 384,095 | 14.3% |
| non object slot | 264,897 | 9.8% |
| out of range | 254,521 | 9.5% |
| metaclass attribute | 187,132 | 6.9% |
| non overriding descriptor | 171,346 | 6.4% |
| class method obj | 152,554 | 5.7% |
| has managed dict | 116,971 | 4.3% |
| mutable class | 112,244 | 4.2% |
| builtin class method | 53,527 | 2.0% |
| shadowed | 3,840 | 0.1% |
| other | 1,669 | 0.1% |
| module attr not found | 1,648 | 0.1% |
| expected error | 591 | 0.0% |
| property | 401 | 0.0% |
This PR
| Name | Count | Self | Cumulative | Miss ratio |
|---|---|---|---|---|
| LOAD_ATTR_CLASS | 88,312,319 | 0.1% | 97.0% | 0.4% |
specialization stats for LOAD_ATTR family
| Kind | Count | Ratio |
|---|---|---|
| unquickened | 190837306 | 3.4% |
| specialization.deferred | 636947625 | 11.3% |
| specialization.deopt | 2321544 | 0.0% |
| hit | 4660987261 | 83.0% |
| miss | 126561331 | 2.3% |
Specialization attempts
| Count | Ratio | |
|---|---|---|
| Success | 4,829,110 | 61.4% |
| Failure | 3,032,191 | 38.6% |
| Failure kind | Count | Ratio |
|---|---|---|
| overridden | 765,516 | 25.2% |
| not managed dict | 582,142 | 19.2% |
| metaclass attribute | 417,214 | 13.8% |
| method | 383,456 | 12.6% |
| non object slot | 265,664 | 8.8% |
| out of range | 253,965 | 8.4% |
| class method obj | 144,317 | 4.8% |
| has managed dict | 117,040 | 3.9% |
| mutable class | 108,085 | 3.6% |
| non overriding descriptor | 74,494 | 2.5% |
| builtin class method | 53,548 | 1.8% |
| shadowed | 3,840 | 0.1% |
| module attr not found | 1,648 | 0.1% |
| expected error | 591 | 0.0% |
| property | 380 | 0.0% |
|
I might prepare a second branch that disallows metaclasses entirely, and compare the results. Again, we should be careful to make sure that |
|
Wow, these are really comprehensive stats. Thanks Brandt. FYI, enum attribute access isn't specialised in 3.11. So whatever we do it probably won't hurt 3.11 that much :). |
|
Disallowing metaclasses entirely (main...brandtbucher:no-metaclasses) gets rid of the 1% slowdown for the benchmark suite as a whole. It looks like it does come with a ~1% hit to the suite's More stats:
specialization stats for LOAD_ATTR family
Specialization attempts
It seems that your earlier intuition was spot-on @Fidget-Spinner (I keep forgetting that 3.11 doesn't specialize class attributes). At this point, I'm thinking that we:
I'm planning to move forward with these today. |
|
Superseded by #94892. |
This patch changes
LOAD_ATTR_CLASSto:__getattribute__It also adds a bunch of regression tests for nasty edge-cases when caching class attributes and methods.