-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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] Precompute set literals for "in" ops against / iteration over set literals #14409
Conversation
constant_fold_expr() replaces my custom logic for string/integer literals and final references. It even supports folding constant expressions which means even {1 + 2} could be precomputed! This means final references to bool, float, bytes*, and complex values are no longer supported, but that's okay for now. In a later patch, we can add proper support for (some of) these types to constant_fold_expr(). *Also bytes are just in general really annoying to work with (because the true bytes value isn't stored anywhere, you have to convert the string representation to bytes instead... ugh) so I don't really care about final refs to bytes values/literals right now. Byte literals IN the sets are still supported.
FYI, I wrote the PR description to be used as the commit message when this is squash merged into main. Please copy and paste it and save yourself some work! :) |
I'll push a functional work around to the mypyc bug that's breaking the build sometime later, but for those wondering "do people really loop over set literals often?" here's a GitHub search query for you (there's even an example in Black's source code 🙂). |
I got tired of trying to compile mypy locally (15 minutes and it's still not done), let's see what CI has to say.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, just a few minor comments. I like that this fixes some important cases where mypyc can generate code slower than CPython.
mypyc/test-data/run-sets.test
Outdated
for i in {None, False, 1, 2.0, "3", b"4", 5j, (6,), CONST}: | ||
s.add(i) | ||
|
||
[file driver.py] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style nit: Instead of having the assertions in driver.py
, could you add them into the main [case ...]
section inside test_*
functions?
I'd like to move towards writing tests this way, since it makes larger test cases easier to read, and there's less risk that contributors forget that code in driver.py
doesn't get compiled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, PTAL 9cdfc98.
Towards mypyc/mypyc#726. (There's a Python compatibility bug that needs to be fixed before the issue can be closed.)
For example, the set literals here are now precomputed as frozensets at module initialization.
Set literal items supported:
irbuild.constant_fold.constant_fold_expr()
None
,True
, andFalse
Results (using gcc-9 on 64-bit Ubuntu)
Master @ 98cc165
This PR
Benchmarks can be found here: mypyc/mypyc-benchmarks#32