bpo-44775: Speed-up typing.cast by implementing it in C - #27474
Conversation
|
@Fidget-Spinner Could you please review this PR? |
| _typing.cast -> object | ||
|
|
||
| typ: object | ||
| val: object |
There was a problem hiding this comment.
If you make the value positional only this couldn't probably be even faster
There was a problem hiding this comment.
Agree it will make cast faster, but in such case it won't be compatible with current version of typing.cast that can accept arguments as keywords.
There was a problem hiding this comment.
Um, does mypy even accept keyword args? I don't see any reason why anyone should use keyword args for cast().
There was a problem hiding this comment.
There was a problem hiding this comment.
Okay then I think the concern is moot.
There was a problem hiding this comment.
If we drop keyword support and make this only METH_FASTCALL, the specializer may eventually make this 8% faster (it currently only does so for CALL_FUNCTION, not CALL_METHOD).
There was a problem hiding this comment.
@uriyyo could you consider removing keyword support please?
For consistency, we can also make the Python typing.cast version keyword-only (we will need to add an entry in What's New and news for that later since it breaks invalid code).
| _typing_cast_impl(PyObject *module, PyObject *typ, PyObject *val) | ||
| /*[clinic end generated code: output=11224a3fa037a9a1 input=4dce97a747d81e0a]*/ | ||
| { | ||
| Py_INCREF(val); |
There was a problem hiding this comment.
I am a bit concerned about this new module where all the functions do the same thing (just return the same value). We should speed up the calls in general as implementing these trivial functions in c to avoid the call overhead feels like a maintainance concern to me.
To be clear, in not opposed to this pr but it starts to feel like a anti-pattern smell.
There was a problem hiding this comment.
I have the same concern with @pablogsal and I would like to request to typing modules authors to follow conventional CPython accelerated extension module structure which is written based on class, function unit not the partial instance method, etc. (This case looks proper as module function unit)
Those extension modules maintain Python and C versions even though maintain cost exists.
And those decisions are based on that the accelerated gain is worth than maintain cost.
So If we feel that following conventional structure is too complicated as the maintenance view,
IMHO, it means that the accelerated version is not worth writing.
For my example, I love to implement vectorcall but I decided to drop my enumerate vectorcall because the implementation was too complicated. (see #25154)
There was a problem hiding this comment.
Anyway, this PR makes 2x faster on my local machine.
Mean +- std dev: [cast_base] 114 ns +- 5 ns -> [cast_pr] 56.0 ns +- 1.8 ns: 2.03x faster
import pyperf
runner = pyperf.Runner()
runner.timeit(name='bench typing.cast',
stmt='val = typing.cast(int, num)',
setup = '''
import typing
import random
num = random.randint(0, 10000)
''')There was a problem hiding this comment.
@pablogsal @corona10 Thanks for your opinion, and I agree with your points.
Case of typing.cast is super simple like it was with typing.NewType.__call__ and we can get x2 boost with small efforts and in my opinion it will be great to have such boost)
I understand your concerns regarding maintenance costs and it's valid point for me.
There was a problem hiding this comment.
And those decisions are based on that the accelerated gain is worth than maintain cost.
@corona10 thank for pinging me. To be honest I don't know enough to comment. Properly benchmarking typing is very hard and still unsolved. IMO, there are 3 aspects to typing performance:
- Static type checkers implemented in Python like mypy.
- Runtime type checkers/introspection like Pydantic.
- Runtime overhead of typed code vs fully untyped code.
All 3 will benefit from general CPython optimizations (eg. Vectorcall, specialization, cache). Although only 3. will benefit the most from speedups to typing module. 2. will benefit a little. 1. will have little benefit.
I'm working on covering these cases in pyperformance (I already submitted 1 open PR for case 1.). And I will send out an email to typing-sig for discussion on 2. and 3. soon.
To be clear, I am not against this PR at all (I'm +0). I just don't have enough data at the moment to decide :(.
There was a problem hiding this comment.
The entire reason _typing exists is that the identity function it houses was deemed improper to put inside _functools or _operator. I think the identity functions have a place as they are next to trivial development-wise but provide a significant performance improvement.
I understand @corona10's argument about providing a full symmetrical accelerated C module vs. a Python module but this currently neither necessary (because it's just two identity functions) nor particularly feasible (because it's a lot of duplicate work and typing is still evolving quite rapidly).
IMO it would be best just to take this improvement as is.
There was a problem hiding this comment.
but this currently neither necessary (because it's just two identity functions) nor particularly feasible (because it's a lot of duplicate work and typing is still evolving quite rapidly).
That is necessary per PEP399, as these functions are going to be the public API
There was a problem hiding this comment.
In particular:
This PEP requires that in these instances that the C code must pass the test suite used for the pure Python code to act as much as a drop-in replacement as reasonably possible.
Given that we are seeking speed, we could drop a bit the requirement and use positional-only, but we are on the line of the interpretation of PEP 399
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
|
I have made the requested changes; please review again |
|
Thanks for making the requested changes! @pablogsal: please review the changes made to this pull request. |
|
This passes |
|
A branch from this comment python/pyperformance#105 (comment): @JelleZijlstra thanks for your insightful thoughts. This PR by Yurii speeds up |
|
Do we have any evidence that this will be faster for 3.11 in real programs? Calls to Python functions will be faster in 3.11. I can't promise how much, probably not enough to close the gap in this case, but it changes the maintenance/performance ratio. [The fastest possible implementation would be to remove the call at compile time. PEP 638 anyone? 🙂] |
|
This PR is stale because it has been open for 30 days with no activity. |
|
@uriyyo can I trouble you to rebase this against main and try the microbenchmarks again please? Recently, we've optimized function calling to inline python calls, so the Python versions of |
|
@Fidget-Spinner Results of my local run: |
Thank you! Can I clarify if that's the pyperf script by Dong-hee at #27474 (comment), or is that from timeit script on your bpo? Either way, the results show that python calls have sped up. It used to be 2x slower 😉 . After a month, I'm now strongly +1 in favor of this. Since @ambv is also in favor (from what I infer, sorry if I'm mistaken), that makes two typing maintainers (I also strongly agree with Łukasz' comments here) . I would love to have syntactic macros over this, but I don't think that's going to land in 3.11. So unless there's anyone strongly -1, I plan to merge this in a week. |
|
@Fidget-Spinner You are right, I used pyperf script by Dong-hee. |
|
The Python version should be even quicker now, and will eventually be faster than the C version 🙂 |
@markshannon the Python version went from 2x slower to 1.85x slower (a huge improvement!). But currently the C version is still quite far ahead. I suspect the Python version may become faster in 3.12/13. So we can accept this change for now and revert it whenever that happens. Users will get to enjoy the speed bump from 3.11 onwards. |
# Conflicts: # Doc/whatsnew/3.11.rst
|
@Fidget-Spinner Could you please review this PR? |
|
I'm still not convinced. Sure, this makes For example, we might want to specialize for all |
|
@Fidget-Spinner do you think this is still worth merging? We should probably make a decision before the feature freeze. For what it's worth, I think it's probably not worth the additional complexity. |
|
@JelleZijlstra I still think it's worth it but with how contentious it is I'd say we shouldn't merge it. Once again, I have no clear insight into how common |
|
Seems like there's not enough support to merge this. Thanks for your contribution, and sorry it took so long! |
|
So no plans to reconsider this, correct? |
https://bugs.python.org/issue44775