GeezerVison is showing github.com — Finish up LOAD_ATTR specialisation · Issue #100288 · python/cpython
Finish up LOAD_ATTR specialisation #100288DescriptionWe should target the following specialisation failures:
- has managed dict
- not in dict (how does this even happen??)
With these two we can hit >90% specialisation successes.
If we're feeling really ambitious, we could aim for "not managed dict" failure too. But we don't need that to achieve >90% successes.
I'm doing the first one. Is anyone interested in investigating the second specialisation failure?
Linked PRs
Activitycorona10 commented on Dec 16, 2022 Oh I am interested in if you don't mind? Fidget-Spinner commented on Dec 16, 2022 Fidget-Spinner commented on Dec 16, 2022 CC @markshannon just these two added specialisations should net us 10% all specialisation failures/successes, bringing us to over 90% specialisation successes. Fidget-Spinner commented on Dec 16, 2022 @corona10 seems like the stats were misleading, not in dict is actually the following:
class C:
i = 0
c = C()
c.i # this fails to specialise
So it's a class attribute lookup, which makes more sense. corona10 commented on Dec 16, 2022 markshannon commented on Dec 16, 2022 Regarding "not managed dict" failures, I think the approach should be to reduce the number of objects that don't have managed dicts, not specialize for them.
We can tweak the management of cached keys, and do some static analysis in the compiler to do that. Fidget-Spinner commented on Jan 4, 2023 Has managed dict isn't worth specialising for - Mark's stats show it has a 40% miss rate and comparing with lats week's stats, it increased the hit rate from 82.0% to 82.5% only. Fidget-Spinner commented on Jan 4, 2023 I have a suspicion we are just about at the point of diminishing returns for LOAD_ATTR. We still have Dong-hee's specialisation for class attributes (that might also be not worth the effort, but it's good to try).
It might not be possible to get LOAD_ATTR hit rates to 90% due to natural polymorphism/dynamism in attribute loads, and I think that's perfectly acceptable. markshannon commented on Jan 4, 2023 The top remaining failures look like this:
| What |
count |
fraction |
| class attr simple |
579,637 |
35.4% |
| has managed dict |
577,900 |
35.3% |
| not managed dict |
258,867 |
15.8% |
"class attr simple" is stuff like
class C:
a = 1
c = C()
c.aWe should be able to handle this fairly easily.
I think the way to deal with the second and third cases is to ensure more objects have values arrays and do not have materialized dicts, not more specializations.
We can still get the specialization rates up to ~95% brandtbucher commented on Jan 26, 2023 I think the remaining cases are just too polymorphic.
For example, adding LOAD_ATTR_CLASS_FROM_INSTANCE to handle the "class attr simple" case barely moves the LOAD_ATTR hit rate (the results are "1.00x faster", and the miss rate for the new opcode is ~32%). We can add it if we want, but I'm just not sure it's worth it. markshannon commented on Jan 27, 2023 Don't forget that additional specializations will provide longer term benefits, not just speed ups in the base interpreter.
More specializations mean better type feedback and potentially longer traces, providing larger regions for optimization for higher tiers.
We should push specialization not just to the point of diminishing returns in terms of speed, but to the edge of negative returns.
I don't think it is worth making the interpreter slower to get more type information, at least not yet, but as long as we aren't slowing things down we should push specialization as far as we can.
The miss rate of 32% seems high, it might be worth a bit of investigation. It might well be polymorphism, but it might be a bug somewhere.
Could you make a (maybe draft) PR, as it sounds like you already have working code? brandtbucher commented on Jan 27, 2023 brandtbucher commented on Jan 27, 2023 Another observation that may help us with the "materialized, managed dict" cases:
I suspect that the vast majority of instance __dict__s are created to be read, or lightly written. Until they overflow the shared keys size or gain a non-unicode key, they still share keys with all of the other virtual dicts (and real __dict__s) for all other instances of the class.
We can probably take advantage of this, right? If we check that dict->ma_keys == heap_type->ht_cached_keys, then its attribute loads can still work just like LOAD_ATTR_INSTANCE_VALUE. The only difference is that we get the values from _PyDictOrValues_GetDict(dorv)->ma_values instead of _PyDictOrValues_GetValues(dorv).
Just an observation I had while looking at this stuff. Seems like it could simplify LOAD_ATTR_WITH_HINT and STORE_ATTR_WITH_HINT quite a bit.
Other notes on LOAD_ATTR_WITH_HINT:
- I'm surprised that we handle non-unicode managed
__dict__s here. It seems like it makes way more sense to just deopt in that case.
- I think it will always fail (
res = ep->me_value; DEOPT_IF(res == NULL, LOAD_ATTR)) for split tables... which is the common case, right?
brandtbucher commented on Jan 27, 2023
I think it will always fail (res = ep->me_value; DEOPT_IF(res == NULL, LOAD_ATTR)) for split tables... which is the common case, right?
...though it looks like it only has a 2.5% miss rate, so I could be missing something here. markshannon commented on Jan 29, 2023 There is some co-design with object layout to consider as well.
- All objects with dicts should use the new layout. Rather than fix
LOAD_ATTR to handle them, we should fix the classes.
- With that done,
LOAD_ATTR_WITH_HINT and STORE_ATTR_WITH_HINT can be removed
- We can't remove uses of
__dict__ in Python code, so we will need the extra instruction you propose.
Can we change object layout a bit, so that a single instruction can handle objects with and without a __dict__?
I don't know of a way to do this without slowing down the common case, but it something to bear in mind.
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|