-
-
Notifications
You must be signed in to change notification settings - Fork 35.3k
bpo-44775: Speed-up typing.cast by implementing it in C #27474
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
Changes from all commits
9e07b2e
79f7bcd
2f48aff
634b506
7d78279
1477a08
3dcbf9e
5cd9a06
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| Speed-up ``typing.cast`` by implementing it in C. Also disallow passing keyword | ||
| arguments to ``typing.cast``. Patch provided by Yurii Karabas. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,9 +27,33 @@ _typing__idfunc(PyObject *module, PyObject *x) | |
| return x; | ||
| } | ||
|
|
||
| /*[clinic input] | ||
| _typing.cast -> object | ||
|
|
||
| typ: object | ||
| val: object | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you make the value positional only this couldn't probably be even faster
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree it will make
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Um, does mypy even accept keyword args? I don't see any reason why anyone should use keyword args for cast().
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay then I think the concern is moot.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @uriyyo could you consider removing keyword support please? For consistency, we can also make the Python |
||
| / | ||
|
|
||
| Cast a value to a type. | ||
|
|
||
| This returns the value unchanged. To the type checker this | ||
| signals that the return value has the designated type, but at | ||
| runtime we intentionally don't check anything (we want this | ||
| to be as fast as possible). | ||
| [clinic start generated code]*/ | ||
|
|
||
| static PyObject * | ||
| _typing_cast_impl(PyObject *module, PyObject *typ, PyObject *val) | ||
| /*[clinic end generated code: output=11224a3fa037a9a1 input=bde696783400a5b0]*/ | ||
| { | ||
| Py_INCREF(val); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Anyway, this PR makes 2x faster on my local machine. 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)
''')
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @pablogsal @corona10 Thanks for your opinion, and I agree with your points. Case of I understand your concerns regarding maintenance costs and it's valid point for me.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@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:
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 :(.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That is necessary per PEP399, as these functions are going to be the public API
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In particular:
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 |
||
| return val; | ||
| } | ||
|
|
||
|
|
||
| static PyMethodDef typing_methods[] = { | ||
| _TYPING__IDFUNC_METHODDEF | ||
| _TYPING_CAST_METHODDEF | ||
| {NULL, NULL, 0, NULL} | ||
| }; | ||
|
|
||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Uh oh!
There was an error while loading. Please reload this page.