Skip to content

Conversation

97littleleaf11
Copy link
Collaborator

Description

Related issue: mypyc/mypyc#644

  • Add a new primitive for Dict.setdefault().
  • Move some dict creation primitives to beginning of dict_ops.py for better code structure.

Test Plan

Please refer to the test-data changes

@97littleleaf11
Copy link
Collaborator Author

Failed in mypyc compiled version.

@TH3CHARLie
Copy link
Collaborator

The compiled mypy fails usually because the newly added primitive breaks sth. Try to comment out the new op and see if the error disappears. If that's the case, you need to double-check if your implementation matches cpython

@97littleleaf11
Copy link
Collaborator Author

@TH3CHARLie thanks! I just found that the second parameter of setdefaultis optional.

@TH3CHARLie
Copy link
Collaborator

Actually for Py_None, you can find none_object in ll_builder to represent that, which means you can build the IR itself without the need for a C wrapper function. But in that way, the two variants of the same op are implemented differently. A better solution would be somehow unify the load_address_op into the op registration.

assert d['c'] == 3
assert d.setdefault('a') == 1
assert d.setdefault('e') == None
assert d.setdefault('e', 100) == None
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also test a subclass of dict that overrides setdefault. We should use the overridden method (example where this probably matters: OrderedDict). You can create a dict subclass that overrides setdefault that behaves differently and use that in the test. If this doesn't work correctly right now, you'll probably need to add a helper similar to CPyDict_KeysView that has a generic fallback implementation for dict subclasses.

@97littleleaf11 97littleleaf11 requested a review from JukkaL April 8, 2021 23:55
Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

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

Thanks for the updates, looks good now! This made a microbenchmark I came up with over 2x faster, which is a great result.

@JukkaL JukkaL merged commit f463a39 into python:master Apr 12, 2021
@97littleleaf11 97littleleaf11 deleted the dict-setdefault branch April 23, 2021 01:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants