Skip to content
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.

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

[mypyc] Faster dict iteration #8725

Merged
merged 24 commits into from
May 6, 2020
Merged

Conversation

ilevkivskyi
Copy link
Member

@ilevkivskyi ilevkivskyi commented Apr 25, 2020

Fixes mypyc/mypyc#167

The implementation is pretty straightforward and follows the idea proposed in the issue. The perf impact is actually pretty small, around 1%

@ilevkivskyi
Copy link
Member Author

Oh, tons of tests fail on Python 3.5 because this apparently changes dict iteration order there. It is probably a good opportunity to fix them (i.e. make them stable).

@ilevkivskyi
Copy link
Member Author

@msullivan I think this should be now ready for review. Some notes:

  • Python 3.5 turned out to be a pain (because of dict ordering), so I just skip this on Python 3.5.
  • I squeeze some ms by having dedicated ops for keys/values/items, if you think it is not worth it, I can return to just one.
  • Would it make sense to also add ops for keys/values/items when the appear in a non-loop context (there are pretty many of these in mypy)? If yes I can either cp them here or make a separate PR.

@JukkaL
Copy link
Collaborator

JukkaL commented Apr 27, 2020

FWIW, I believe that the most common uses of keys/values/items outside of a loop context in general Python code is as arguments to list (and maybe set and/or dict), such as list(foo.values()). If special casing helps these use cases (at least for small dictionaries), I think that it's worth supporting non-loop contexts. Separate PR would be fine.

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.

Nice, this will speed up some very common operations. In a simple microbenchmark this made iteration about 30% faster. Left a few minor comments.

mypyc/test-data/run.test Outdated Show resolved Hide resolved
mypyc/test/test_run.py Outdated Show resolved Hide resolved
mypyc/irbuild/for_helpers.py Outdated Show resolved Hide resolved
@ilevkivskyi
Copy link
Member Author

@msullivan So I disable the fast path for subclasses now (and enable on Python 3.5). It looks pretty complicated, but not too bad probably. Please let me know if it looks OK.

@JukkaL
Copy link
Collaborator

JukkaL commented May 5, 2020

This looks good to me. Let's merge this tomorrow unless @msullivan has some additional feedback.

@ilevkivskyi
Copy link
Member Author

This looks good to me. Let's merge this tomorrow unless @msullivan has some additional feedback.

I am assuming Sully doesn't have any comments here, so will merge now.

@ilevkivskyi ilevkivskyi merged commit 781dd69 into python:master May 6, 2020
@ilevkivskyi ilevkivskyi deleted the mypyc-dict-iter branch May 6, 2020 12:07
ilevkivskyi added a commit that referenced this pull request May 6, 2020
This covers both view and list versions (it looks like both are used relatively often, at least in mypy). This accompanies #8725 to account for cases where `keys`/`values`/`items` appear in non-loop contexts.
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.

Faster dict iteration
2 participants