GeezerVison is showing github.com — decimal: Use Argument Clinic · Issue #73487 · python/cpython
decimal: Use Argument Clinic #73487Description
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
assignee = 'https://github.com/skrah'
closed_at = <Date 2018-05-29.22:14:26.039>
created_at = <Date 2017-01-17.16:38:44.176>
labels = ['3.7', 'performance']
title = 'decimal: Use FASTCALL and/or Argument Clinic'
updated_at = <Date 2019-03-21.00:24:41.547>
user = 'https://github.com/vstinner' bugs.python.org fields:
activity = <Date 2019-03-21.00:24:41.547>
actor = 'vstinner'
assignee = 'skrah'
closed = True
closed_date = <Date 2018-05-29.22:14:26.039>
closer = 'vstinner'
components = []
creation = <Date 2017-01-17.16:38:44.176>
creator = 'vstinner'
dependencies = []
files = ['46320', '46435']
hgrepos = []
issue_num = 29301
keywords = ['patch']
message_count = 23.0
messages = ['285665', '285670', '285671', '285673', '285680', '286359', '286368', '286369', '286371', '286372', '286374', '286375', '286448', '287131', '287133', '287149', '297135', '297153', '318123', '338430', '338462', '338513', '338516']
nosy_count = 5.0
nosy_names = ['rhettinger', 'vstinner', 'skrah', 'serhiy.storchaka', 'malin']
pr_nums = []
priority = 'normal'
resolution = 'out of date'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'performance'
url = 'https://bugs.python.org/issue29301'
versions = ['Python 3.7'] Linked PRs
Activityvstinner commented on Jan 17, 2017 I'm currently working on the isuse bpo-29259: "Add tp_fastcall to PyTypeObject: support FASTCALL calling convention for all callable objects". I used bm_telco of the performance benchmark suite to check which functions still require to create a temporary tuple for positional arguments and a temporary dict for keyword arguments. I found 3 remaining functions which have an impact on the result of the benchmark:
- print(): optimized by the issue bpo-29296
- _struct.unpack(): I just created the issue bpo-29300 "Modify the _struct module to use FASTCALL and Argument Clinic"
- _decimal.Decimal.quantize()
I would like to know if Stephan would be ok to modify the _decimal module to use FASTCALL. I know that recently he reverted changes to keep the same code base on Python 3.5, 3.6 and 3.7.
With 4 changes (tp_fastcall bpo-29259, print bpo-29296, unpack bpo-29300 and this issue), bm_telco becomes 22% faster which is not negligible!
20.9 ms +- 0.5 ms => 16.4 ms +- 0.5 ms
Attached decimal.patch patch is the strict minimum change to optimize bm_telco, but I would prefer to change all _decimal functions and methods using METH_VARARGS and METH_VARARGS|METH_KEYWORDS to convert them to METH_FASTCALL.
The best would be to use Argument Clinic. AC exists since Python 3.5, so it should be possible to keep the same code base on Python 3.5-3.7, only generated code would be different. vstinner commented on Jan 17, 2017 Oh wait, I'm not sure that attached patch has a significant impact on performances. It seems like the speedup mostly comes from the print patch:
http://bugs.python.org/issue29296#msg285668
But well, it is still interesting to use METH_FASTCALL in decimal ;-) serhiy-storchaka commented on Jan 17, 2017 rhettinger commented on Jan 17, 2017 This should not go in without Stephan Krah's approval. This code is finely tuned and carefully arranged. vstinner commented on Jan 17, 2017 Raymond Hettinger: "This should not go in without Stephan Krah's approval. This code is finely tuned and carefully arrang"
Sure, that's why I opened this issue.
Serhiy Storchaka: "See msg207652 in bpo-20177."
Oh, I didn't know that Stefan Krah already gave his opinion.
Since the performance impact is unclear, I prefer to close this issue. I will come back if argument parsing is more clearly identified as a performance bottleneck. vstinner commented on Jan 27, 2017 Hello, I'm back! :-)
I almost abandonned my the tp_fastcall change (bpo-29259).
print() now uses FASTCALL (bpo-29296), it already made bm_telco faster:
https://speed.python.org/timeline/#/?exe=5&ben=telco&env=1&revs=50&equid=off&quarts=on&extr=on
(see the speedup around January 16 at the right)
For the unpack module, I'm still working on my patch in the issue bpo-29300, but it doesn't seem to have a significant impact on bm_telco.
So the remaining question is the usage of FASTCALL or Argument Clinic in the _decimal module.
Since the performance impact is unclear, I prefer to close this issue. I will come back if argument parsing is more clearly identified as a performance bottleneck.
I ran a new benchmark and FASTCALL makes bm_telco benchmark 1.11x faster:
Median +- std dev: [ref] 19.4 ms +- 0.7 ms -> [decimal-2.patch] 17.5 ms +- 0.6 ms: 1.11x faster (-10%)
So I decided to reopen the issue.
Stefan: what do you think of using Argument Clinic and/or FASTCALL in _decimal? Is "bm_telco 1.11x faster" significant enough for you to justify such change?
"The best would be to use Argument Clinic. AC exists since Python 3.5, so it should be possible to keep the same code base on Python 3.5-3.7, only generated code would be different." serhiy-storchaka commented on Jan 27, 2017 I would dissuade from direct using of FASTCALL, but Argument Clinic is enough reliable. I would recommend to use Argument Clinic unless Stefan supports external fork of the decimal module and is limited by public API. vstinner commented on Jan 27, 2017 Oops, I forgot to attach decimal-2.patch, here you have.
I would dissuade from direct using of FASTCALL, but Argument Clinic is enough reliable.
I wrote decimal-2.patch in a few minutes, when I was trying to find all functions still calling _PyStack_AsTuple() in my tp_fast{new,init,call} fork. I just wrote it to identify which parts of CPython can be optimized to make bm_telco faster.
I agree that Argument Clinic should be preferred over using directly METH_FASTCALL, especially for the _decimal module: Stefan already wrote that he wants to have the same code base on Python 3.5, 3.6 and 3.7.
I would recommend to use Argument Clinic unless Stefan supports external fork of the decimal module and is limited by public API.
What is the status of Argument Clinic? Is it something "public" or not?
The status of _decimal is also unclear to me. Is it part of CPython or not? :-) I know that there is also a "backported" module for Python 2:
https://pypi.python.org/pypi/cdecimal
http://www.bytereef.org/mpdecimal/index.html
IMHO it's a great tool. Maybe it's time to make it usable outside CPython in Python 3.7? Or maybe we should wait until the remaining missing features are implemented? Issues bpo-20291 and bpo-29299 for example.
I'm now waiting for Stefan's feedback ;-) skrah commented on Jan 27, 2017 I'll take a look when I have the opportunity.
AC will not happen: It makes the module too large and unreadable. serhiy-storchaka commented on Jan 27, 2017
What is the status of Argument Clinic? Is it something "public" or not?
No, it is for for internal CPython use only. It lacks some features (support
of var-positional and var-keyword parameters, optional parameters without
default value), the syntax of positional-only parameters is not officially
accepted still, and future optimizations can require incompatible changes.
Only when all CPython builtins and extensions be converted to Argument Clinic,
PEP-457 (or an alternative) be accepted, Argument Clinic issues be resolved,
we could say it stable enough. For now even Argument Clinic tests are broken. skirpichev commented on Aug 22, 2025 With #138015 - conversion will be finished.
Some things that left out so far:
- conversion of getters/setters (this stuff is undocumented so far)
- use defining_class AC converter
- something else?
I would appreciate feedback.
The pt.2 shows slight performance boost, I'm not sure it worth:
| Benchmark |
ref |
patch |
| Decimal(123).to_integral_value() |
605 ns |
593 ns: 1.02x faster |
Details
diff --git a/Modules/_decimal/_decimal.c b/Modules/_decimal/_decimal.c
index de7200af8c..0c5bd43361 100644
--- a/Modules/_decimal/_decimal.c
+++ b/Modules/_decimal/_decimal.c
@@ -4042,6 +4042,7 @@ _decimal_Decimal_as_integer_ratio_impl(PyObject *self)
/*[clinic input]
_decimal.Decimal.to_integral_value
+ cls: defining_class
rounding: object = None
context: object = None
@@ -4053,15 +4054,16 @@ rounding mode of the current default context is used.
[clinic start generated code]*/
static PyObject *
-_decimal_Decimal_to_integral_value_impl(PyObject *self, PyObject *rounding,
+_decimal_Decimal_to_integral_value_impl(PyObject *self, PyTypeObject *cls,
+ PyObject *rounding,
PyObject *context)
-/*[clinic end generated code: output=7301465765f48b6b input=04e2312d5ed19f77]*/
+/*[clinic end generated code: output=23047d848ef84db1 input=85aa9499a21ea8d7]*/
{
PyObject *result;
uint32_t status = 0;
mpd_context_t workctx;
- decimal_state *state = get_module_state_by_def(Py_TYPE(self));
+ decimal_state *state = PyType_GetModuleState(cls);
CONTEXT_CHECK_VA(state, context);
workctx = *CTX(context);encukou commented on Aug 28, 2025 Thank you!
IMO, defining_class is best practice, for methods that can use it. PyType_GetModuleState is a lookup that can be avoided.
I don't think we need AC for getters & setters. skirpichev commented on Aug 28, 2025 Ok, then #138221 should finish conversion. vstinner commented on Sep 18, 2025
Ok, then #138221 should finish conversion.
I merged this PR. Can we close this issue? skirpichev commented on Sep 18, 2025 Reacted by Victor Stinner, Adam Turner and Erlend E. Aasland
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|