Repository navigation
Finish up LOAD_ATTR specialisation #100288
Description
Activity
Oh I am interested in if you don't mind?
Sure! Go ahead!
CC @markshannon just these two added specialisations should net us 10% all specialisation failures/successes, bringing us to over 90% specialisation successes.
@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 specialiseSo it's a class attribute lookup, which makes more sense.
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.
- added 3 commits that reference this issue
on Dec 23, 2022 - added a commit that references this issue
on Dec 28, 2022 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.
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_ATTRhit rates to 90% due to natural polymorphism/dynamism in attribute loads, and I think that's perfectly acceptable.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.a
We 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%
- added 2 commits that reference this issue
on Jan 5, 2023 I think the remaining cases are just too polymorphic.
For example, adding
LOAD_ATTR_CLASS_FROM_INSTANCEto handle the "class attr simple" case barely moves theLOAD_ATTRhit 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.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?Here you go: #101379.
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 likeLOAD_ATTR_INSTANCE_VALUE. The only difference is that we get the values from_PyDictOrValues_GetDict(dorv)->ma_valuesinstead of_PyDictOrValues_GetValues(dorv).Just an observation I had while looking at this stuff. Seems like it could simplify
LOAD_ATTR_WITH_HINTandSTORE_ATTR_WITH_HINTquite 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?
- I'm surprised that we handle non-unicode managed
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.
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_ATTRto handle them, we should fix the classes. - With that done,
LOAD_ATTR_WITH_HINTandSTORE_ATTR_WITH_HINTcan 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.
- All objects with dicts should use the new layout. Rather than fix
- added a commit that references this issue
on Jan 31, 2023
We should target the following specialisation failures:
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
LOAD_ATTR#101354