py: Implement __dict__ for instances

[stinos]

No description provided.

[pfalcon]

What's up with those parentheses?

[pfalcon]

Though wait, while all this creation of copies, why not reuse self->members?

[stinos]

I'm not sure how. Is it ok to create a new mp_obj_dict_t, then use mp_map_init_fixed_table(&dict->map, self->members.used, self->members.table) on it?

[stinos]

Ok so I gave reuse a go but it does not seem safe, maybe because it's not the correct way but I don't immediately see another way:

    if (attr == MP_QSTR___dict__) {
        mp_obj_t dict_obj = mp_obj_new_dict(0);
        mp_obj_dict_t *dict = MP_OBJ_TO_PTR(dict_obj);
        mp_map_init_fixed_table(&dict->map, self->members.used, MP_OBJ_FROM_PTR(self->members.table));
        dict->map.is_ordered = self->members.is_ordered;
        dict->map.alloc = self->members.alloc;
        dict->map.all_keys_are_qstrs = self->members.all_keys_are_qstrs;
        dest[0] = dict_obj;
        return;
    }

Through the dict returned class members can now be modified. However what seems 'unsafe' to me:

• I thought since map->is_fixed is 1, it would prohibit anything being added/removed from the dict but that is not the case (there is logic to do so in mp_map_lookup but that only kicks in if is_ordered == 0). As such members can also be added and removed (yay, like CPython) but this results in used/alloc from dict->map being updated, whereas self->members' used/alloc are not changed even though internally the point to the same table. That cannot be right.
• there's a function mp_map_free which does not seem used, but if it were somehow called on dict->map and self->members things would go bad

[dpgeorge]

I thought since map->is_fixed is 1, it would prohibit anything being added/removed from the dict but that is not the case (there is logic to do so in mp_map_lookup but that only kicks in if is_ordered == 0).

That doesn't sound right... I think the reason it's like that is because (up until now) only ordered maps could be fixed (ie fixed implied ordered). But that won't be the case if #1731 is used. So it should be changed so that fixed is independent of ordered. Then probably your new way would work.

[dpgeorge]

is_fixed problem fixed in 6dde019.

[stinos]

Thanks! This introduces a bunch of possible NULL dereferencing locations since functions like dict_update/dict_store do not check the return value of mp_map_lookup. Changing those to check the value just to accomodate this PR doesn't seem right - moreover it would create a dict which isn't really a dict but rather some cross-breed readonly dict which advertises itself as a dict.. Leaving it as-is feels even more wrong because of the same problem (not really a dict) but it adds segfaulting on top. Not really sure what to do here. Possible options I see:

1. Stick with original implementation
2. Reuse self->members and assume users know the dict returned should not be altered
3. Reuse self->members and add checks in dict functions to deal with underlying is_fixed map (i.e. mp_map_lookup might return NULL) so the dict can effectively not be altered
4. Resuse self->members but return a dict_view instead of dict itself (pro: effectively read-only, con: different interface than dict unless methods are added)
5. Refactor mp_obj_dict_t to have a pointer to a map and just set it to self->members (pro: is actually a rather simple solution and AFAICS solves all problems, including full CPython compatibility, con: ??)

@dpgeorge @pfalcon @dhylands please comment

[pfalcon]

@stinos : Right, I missed that mp_obj_dict contains map, not references it. I'd still think that just making a clone of a map with a couple of memcpy() would be less code bytes than recreating it by inserting, but if you think it's not worth it, let's be back to the original patch.

[dhylands]

Refactor mp_obj_dict_t to have a pointer to a map and just set it to self->members (pro: is actually a rather simple solution and AFAICS solves all problems, including full CPython compatibility, con: ??)

Another potential solution would be to make self->members be a dict and have __dict__ just return that.

I did some checks and what I see for memory usage is the following:
sizeof(mp_obj_instance_t) = 16 + 4 * num-sub-objects
sizeof(mp_obj_dict_t) = 16
sizeof(mp_map_t) = 12

I think that means that instance of objects which are declared class Foo: take up 1 heap block, and instance of objects declared class Foo(object) take up 2.

If we changed mp_obj_dict_t to use a pointer to a map, then every dictionary would take up 2 heap blocks, where it currently only takes up 1.

If we changed mp_obj_instance_t to contain a dict rather than a map, then instances of objects declared class Foo: would now take up 2 heap blocks, but instances of objects declared class Foo(object): would still only take up 2 heap blocks.

[pfalcon]

Ok, let's have this merged and move on. Note that trivial changes/optimizations applied and test added.

[dpgeorge]

Note that trivial changes/optimizations applied and test added

The original PR included a new test... should we merge both that and the new one into a single file?

[stinos]

Both tests do basically the same so I'd just keep the one with the most suitable name? @dpgeorge object_dict or builtin_dict?

[dpgeorge]

@stinos I'd expect builtin_dict to be a test for the builtin dict type, so let's stick with object_dict for __dict__ tests.