8000 py: Implement __dict__ for instances by stinos · Pull Request #1757 · micropython/micropython · GitHub
[go: up one dir, main page]
Skip to content
8000

py: Implement __dict__ for instances #1757

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

py: Implement __dict__ for instances #1757

wants to merge 1 commit into from

Conversation

stinos
Copy link
Contributor
@stinos stinos commented Dec 29, 2015

No description provided.

@stinos
py: Implement __dict__ for instances
7cd9edc 
Note that even though wrapped in MICROPY_CPYTHON_COMPAT, it is not
fully compatible because the modifications to the dictionary do not
propagate to the actual instance members.
pfalcon
pfalcon reviewed Dec 30, 2015
View reviewed changes
py/objtype.c
// Create a new dict with a copy of the instance's map items.
// This creates, unlike CPython, a 'read-only' __dict__: modifying
// it will not result in modifications to the actual instance members.
mp_map_t *map = &(self->members);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's up with those parentheses?

@pfalcon
Copy link
Contributor
pfalcon commented Dec 30, 2015

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

@stinos
Copy link
Contributor Author
stinos commented Dec 30, 2015

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
Copy link
Contributor Author
stinos commented Dec 30, 2015

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 dpgeorge mentioned this pull request Dec 31, 2015
Closed
@dpgeorge
Copy link
Member

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
Copy link
Member

is_fixed problem fixed in 6dde019.

@stinos
Copy link
Contributor Author
stinos commented Dec 31, 2015

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
Copy link
Contributor
pfalcon commented Dec 31, 2015

@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
Copy link
Contributor

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
Copy link
Contributor
pfalcon commented Jan 3, 2016

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

@pfalcon pfalcon closed this Jan 3, 2016
@dpgeorge
Copy link
Member
dpgeorge commented Jan 3, 2016

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
Copy link
Contributor Author
stinos commented Jan 13, 2016

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
Copy link
Member

@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.

@stinos stinos mentioned this pull request Jan 14, 2016
Closed
@stinos stinos deleted the builtin-dict branch January 15, 2016 13:29
@dpgeorge dpgeorge mentioned this pull request Jun 1, 2016
Closed
@touilleMan touilleMan mentioned this pull request Feb 6, 2017
Closed
tannewt added a commit to tannewt/circuitpython that referenced this pull request Apr 9, 2019
@tannewt
Merge pull request micropython#1757 from pewpew-game/displayio-bus
672a102 
Expose displayio.Display.bus
@stinos stinos mentioned this pull request Jun 10, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@stinos @pfalcon @dpgeorge @dhylands
0