[mypyc] feat: extend stararg fastpath from #19623 to handle lists and generic sequences #19629
Conversation
|
I pulled in the latest changes on master, the diff is now cleaned up and this PR is ready for review. |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
b13aaa2 to
de18a19
Compare
| @@ -3546,3 +3540,266 @@ L0: | |||
| r2 = PyObject_Vectorcall(r1, 0, 0, 0) | |||
| r3 = box(None, 1) | |||
| return r3 | |||
|
|
|||
| [case testStarArgFastPathTuple] | |||
There was a problem hiding this comment.
Can you add run tests corresponding to these irbuild test cases? Please try to look for existing run tests where you can add new def test_foo functions, or add a new test case that has no driver and multiple def test_foo functions that correspond to individual test cases, since each top-level run test case has significant per-test-case overhead.
No I did not, I typically only do real benchmarking in cases that are more ambiguous as to whether or not the change is truly beneficial. In cases where it's clearly beneficial but not clear how much, I usually only benchmark when I'm abnormally curious about the result. Is that a blocker for this one? |
No. I ran a quick micro-benchmark to just get a general idea of whether the performance impact is likely visible. In this case performance improved by 1.6x, so it looks good. (We've had cases were an apparent optimization actually made code slower, so it's a good idea to do at least minimal benchmarking if there is any doubts about the impact.) Many optimizations aren't visible in (most) larger-scale benchmarks, since the noise floor tends to be 0.5% or more, so any improvement below 0.5% can't be easily measured. |
This PR extends #19623 with additional logic for handling non-tuple star inputs
Now, we can use the fast path for any arbitrary sequence, in addition to tuples.
I opted to separate this PR from 19623 to keep them smaller and easier to review, and to declutter the changes in the IR.
This PR is ready for review (after #19623 is reviewed and merged)