Refactor to use match case where it make sense following python 3.9 drop#13727
Conversation
Pierre-Sassoulas
added
the
skip news
| pass | ||
| else: | ||
| break | ||
| match item: |
There was a problem hiding this comment.
this looks interesting - whats the performance impact - i would expect match to be faster but that may be wishful thinking
There was a problem hiding this comment.
Unfortunately we had mixed results in pylint regarding performance. What do you suggest as a benchmark ?
There was a problem hiding this comment.
We need code fragments that trigger the different cases and then we need a codegen that given a number creates that many instances
Then we can observe/profile
Im not sure whether we should synthesize a ast or just text
There was a problem hiding this comment.
A preliminary benchmark (done by claude) is not very encouraging : https://claude.ai/public/artifacts/3d4158ea-0594-4442-8b4c-975bc5a54ce1 (14% slower on my side, python 3.12 / ubuntu)
There was a problem hiding this comment.
Seems worse than i hoped
There was a problem hiding this comment.
The examples I took where specifically the one where the diff was very favorable (There's a lot of match on raw string that could be done but the performance is worse in this case as there's no index lookup creation), so I also expected better. From what I read it should be a neutral change performance wise except if isinstance were smartly grouped together with and / or to do less checks. Maybe the regression is due to microbenchmarking, maybe match is only a readability change. It's hard to find information that is not slop about this topic. (found this for example : https://discuss.python.org/t/pattern-matching-optimization-comparison-of-values-specified-through/20791)
There was a problem hiding this comment.
But comparing bytecode seems like something to potentially use fir understanding our cases
There was a problem hiding this comment.
Marc Mueller dug into the CPython implementation here (pylint specific but still relevant) : pylint-dev/pylint#10544 (comment)
There was a problem hiding this comment.
TLDR: the faster you want it to be the shittier it have to look:
- case ast.Expr(value=ast.Constant(value=str(doc))) if expect_docstring:
+ case ast.Expr(value=ast.Constant(value=str() as doc)) if expect_docstring:... should be faster, etc. (isinstance is very well optimized)
There was a problem hiding this comment.
Opened an issue for CPython: python/cpython#138912
|
This is definitely more readable. |
Pierre-Sassoulas
force-pushed
the
match-case-goodies
branch
from
7471ede to
72afed6
Compare
nicoddemus
left a comment
nicoddemus
left a comment
There was a problem hiding this comment.
LGTM! Thanks @Pierre-Sassoulas!
src/_pytest/assertion/rewrite.py
Outdated
| # mypy's false positive, we're checking that the 'target' attribute exists. | ||
| v.left.target.id = pytest_temp # type:ignore[attr-defined] |
There was a problem hiding this comment.
Would probably be fixed with: python/mypy#19736
Pierre-Sassoulas
force-pushed
the
match-case-goodies
branch
2 times, most recently
from
59bf241 to
9653a42
Compare
|
I added a test to fix the coverage failure. What do you think about the compromise between readability and performance @RonnyPfannschmidt ? |
|
I like the readability Im wondering if mypyc would generate a win here |
|
I'm not sure I understand the mypyc comment, what are your suggesting exactly ? |
|
As would compiler of the rewriter get us reasonable speedup |
|
I believe @RonnyPfannschmidt is just wondering aloud what mypyc would generate from that code, not suggesting to actually use mypyc in some way here. @Pierre-Sassoulas from my POV please feel free to go ahead and merge! 😁 |
Pierre-Sassoulas
force-pushed
the
match-case-goodies
branch
from
52b6fd9 to
7308a72
Compare
5 participants
Follow-up to #13724, will need a rebase on it to work (python 3.9 still in the CI on this branch).