LOAD_ATTR/METHOD deopts on dict versions too quickly

[Fidget-Spinner]

Edit: this was previously titled "Dictkeys version should be tied to dict state. However, I realised I messed up and was focusing on the wrong aspect of the problem. See #431 (reply in thread)

Currently, we invalidate and increment dict keys version for a type every time one of its object adds/removes an attribute.

>>> class A: ...
...
>>> a = A()
>>> get_dk_version(a)
<Shared keys> 62
>>> get_dk_version(A)
63
>>> get_dk_version(A())
<Shared keys> 62
>>> a.a = 1
>>> get_dk_version(a)
<Shared keys> 64
>>> get_dk_version(A())
<Shared keys> 64

In the above example, we don't need to update the dictkeys version of A(). We should just let it keep its old version. Dictkeys should be tied to individual state.

The benefit of this is that our method/attribute specialisations can handle more dynamic code. Especially those that modify attributes frequently.

Currently, I'm not too sure what approach we need to achieve this. The current behavior is a side effect of the key sharing nature of dictionaries and it's very memory efficient. So anything modifying that may cause CPython to consume more memory.

If we were to implement hidden classes. This would be significantly easier as it would just be a version tied to the hidden class. However, I understand from #6 that hidden classes are not being pursued.

[markshannon]

How have you defined get_dk_version?

Why would we want hidden classes for Python? We have real classes.

[Fidget-Spinner]

I'm not advocating for hidden classes. I was just throwing out a hypothetical scenario :). This is get_dk_version.

static PyObject *
get_dk_version(PyObject *self, PyObject *owner)
{
    PyTypeObject *owner_cls = Py_TYPE(owner);
    PyObject *dict;
    PyDictKeysObject *keys;
    if (owner_cls->tp_flags & Py_TPFLAGS_MANAGED_DICT) {
        printf("<Shared keys> ");
        dict = *_PyObject_ManagedDictPointer(owner);
        keys = ((PyHeapTypeObject *)owner_cls)->ht_cached_keys;
    }
    else {
        Py_ssize_t dictoffset = owner_cls->tp_dictoffset;
        PyObject *dict = *(PyObject **)((char *)owner + dictoffset);
        if (dictoffset <= 0 || dict == NULL) {
            Py_RETURN_NONE;
        }
        keys = ((PyDictObject *)dict)->ma_keys;
    }
    uint32_t keys_version = _PyDictKeys_GetVersionForCurrentState(keys);
    return PyLong_FromLong(keys_version);
}

[markshannon]

Doesn't dict = *_PyObject_ManagedDictPointer(owner); segfault for objects with virtual dicts? It might not with -O3 as the pointer dereference might be eliminated.

[Fidget-Spinner]

Sorry, but what do you mean by a virtual dict? Are you referring to the layout with a lazy __dict__ object?

[markshannon]

Yes

[markshannon]

I don't understand what is the problem here.

>>> a = A()
>>> get_dk_version(a)
<Shared keys> 62

>>> a.a = 1
>>> get_dk_version(a)
<Shared keys> 64

This seems to be exactly what we want.

[markshannon]

The keys have changed, you've added "a".

[Fidget-Spinner]

Wait sorry I think I miscommunicated the idea. I realised I shouldn't be focusing on the dict keys. What I was trying to emphasize is that LOAD_METHOD/ATTR use these key versions. In the above scenario, LOAD_METHOD/ATTR would deopt on all other instances of A due to the changed key versions. However, except for the object a, there's no reason for it to deopt as the specialisations are still perfectly valid.

The title of this discussion should instead be talking about specialisation misses of LOAD_ATTR/METHOD rather than about dict keys version.

[markshannon]

A dict with a key "foo", can shadow a method "foo". So the difference in the keys is important.

class C:
     def foo(self): return 1

>>> c = C()
>>> c.foo()
1
>>> c.foo = int
>>> c.foo()
0

LOAD_ATTR_PROPERTY doesn't check the dict keys, because properties are overriding descriptors.

[Fidget-Spinner]

A dict with a key "foo", can shadow a method "foo". So the difference in the keys is important.

Yes. I wasn't referring to dropping keys version. This the behavior I don't want:

>>> class A: ...
...
>>> a1 = A()
>>> get_dk_version(a1)
<Shared keys> 62
>>> a2 = A()
>>> get_dk_version(a2)
<Shared keys> 62
>>> a1.x = 1 # This updates a2 dict keys version too
>>> get_dk_version(a1)
<Shared keys> 63
>>> get_dk_version(a2)  # Now a2 has to deopt method lookups, even though nothing about it changed!
<Shared keys> 63

[markshannon]

I don't see this as a problem. It will re-optimize again soon enough.
We are specializing for the class as a whole, not particular values.

Occasionally some attributes will get added late. We either temporarily de-optimize, or lose the ability to optimize method calls across classes where some instances gain an additional attribute.