decimal: Use Argument Clinic

[vstinner]

BPO	29301
Nosy	@rhettinger, @vstinner, @skrah, @serhiy-storchaka, @animalize
Files	decimal.patch decimal-2.patch

^{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

• gh-73487: Convert _decimal to use Argument Clinic (part 1) #137606
• gh-73487: Convert _decimal to use Argument Clinic (part 2) #137637
• gh-73487: Convert _decimal to use Argument Clinic (part 3) #137844
• gh-73487: Convert _decimal to use Argument Clinic (part 4) #137931
• gh-73487: Convert _decimal to use Argument Clinic (part 5) #137948
• gh-73487: Convert _decimal to use Argument Clinic (part 6) #138015
• gh-73487: Convert _decimal to use Argument Clinic (part 7) #138221

[vstinner]

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]

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]

See msg207652 in bpo-20177.

[rhettinger]

This should not go in without Stephan Krah's approval. This code is finely tuned and carefully arranged.

[vstinner]

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]

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]

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]

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]

I'll take a look when I have the opportunity.

AC will not happen: It makes the module too large and unreadable.

[serhiy-storchaka]

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.