Use METH_FASTCALL in str methods #73472

vstinner · 2017-01-16T17:30:52Z

BPO	29286
Nosy	@vstinner, @larryhastings, @methane, @serhiy-storchaka, @1st1
PRs	bpo-30524: Fix _PyStack_UnpackDict() #1886
Files	unicode_allow_kw.patch ac_more_fastcalls.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 = None
closed_at = <Date 2017-02-01.16:45:08.895>
created_at = <Date 2017-01-16.17:30:51.705>
labels = ['3.7', 'performance']
title = 'Use METH_FASTCALL in str methods'
updated_at = <Date 2017-06-09.11:24:56.334>
user = 'https://github.com/vstinner'

bugs.python.org fields:

activity = <Date 2017-06-09.11:24:56.334>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2017-02-01.16:45:08.895>
closer = 'vstinner'
components = []
creation = <Date 2017-01-16.17:30:51.705>
creator = 'vstinner'
dependencies = []
files = ['46305', '46308']
hgrepos = []
issue_num = 29286
keywords = ['patch']
message_count = 20.0
messages = ['285574', '285575', '285576', '285578', '285581', '285584', '285587', '285594', '285595', '285596', '285597', '285599', '285600', '285614', '286647', '286648', '286649', '286650', '286653', '295516']
nosy_count = 6.0
nosy_names = ['vstinner', 'larry', 'methane', 'python-dev', 'serhiy.storchaka', 'yselivanov']
pr_nums = ['1886']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'performance'
url = 'https://bugs.python.org/issue29286'
versions = ['Python 3.7']

vstinner · 2017-01-16T17:30:51Z

Changes 27dc9a1c061e and 01b06ca45f64 converted the str (Unicode) methods to Argument Clinic, cool! But str methods taking more than one argument use positional-only arguments.

Currently, Argument Clinic doesn't use METH_FASTCALL for these methods, but METH_VARARGS.

There are two options:

Allow passing arguments as keywoards: str.replace(old='a', new='b')
Enhance Argument Clinic to use also METH_FASTCALL for functions using positional-only functions

The goal is to speedup method calls. Example with str.replace():

$ ./python-patch -m perf timeit '"a".replace("x", "y")' --duplicate=100 --compare-to ./python-ref
python-ref: ..................... 132 ns +- 1 ns
python-patch: ..................... 102 ns +- 2 ns

Median +- std dev: [python-ref] 132 ns +- 1 ns -> [python-patch] 102 ns +- 2 ns: 1.30x faster (-23%)

vstinner · 2017-01-16T17:31:49Z

Background: see also the old issue bpo-17170 closed as rejected: "string method lookup is too slow".

vstinner · 2017-01-16T17:32:53Z

The issue bpo-29263 "Implement LOAD_METHOD/CALL_METHOD for C functions" should also optimize C methods even more.

1st1 · 2017-01-16T17:54:46Z

Allow passing arguments as keywoards: str.replace(old='a', new='b')

I think Guido explicitly stated that he doesn't like the idea to always allow keyword arguments for all methods. I.e. str.find('aaa') just reads better than str.find(needle='aaa'). Essentially, the idea is that for most of the builtins that accept one or two arguments, positional-only parameters are better.

Enhance Argument Clinic to use also METH_FASTCALL for functions using positional-only functions

So this is the option to go with.

serhiy-storchaka · 2017-01-16T18:52:37Z

What is python-patch?

vstinner · 2017-01-16T20:00:59Z

What is python-patch?

It's Python patched with attached unicode_allow_kw.patch: allow to pass parameters as keywords for Unicode methods.

methane · 2017-01-16T23:24:01Z

Parsing positional arguments for METH_KEYWORDS and METH_FASTCALL can be faster by
http://bugs.python.org/issue29029

methane · 2017-01-17T00:09:26Z

This patch makes AC produces more FASTCALL instead of VARARGS.

When looking AC output, one downside is it produces many consts like:
+ static const char * const _keywords[] = {"", "", "", "", "", NULL};

vstinner · 2017-01-17T01:10:05Z

ac_more_fastcalls.patch: Hum, I guess that your change on _PyStack_UnpackDict() is related to a bug in the function prototype. The function is unable to report failure if args is NULL. It changed the API in the change 38ab8572fde2.

python-dev · 2017-01-17T01:17:08Z

New changeset d07fd6e6d449 by Victor Stinner in branch 'default':
Rename _PyArg_ParseStack to _PyArg_ParseStackAndKeywords
https://hg.python.org/cpython/rev/d07fd6e6d449

New changeset 01c57ef1b651 by Victor Stinner in branch 'default':
Add _PyArg_ParseStack() helper function
https://hg.python.org/cpython/rev/01c57ef1b651

New changeset 8bfec37ea86a by Victor Stinner in branch 'default':
Add _PyArg_NoStackKeywords() helper function
https://hg.python.org/cpython/rev/8bfec37ea86a

New changeset 38ab8572fde2 by Victor Stinner in branch 'default':
_PyStack_UnpackDict() now returns -1 on error
https://hg.python.org/cpython/rev/38ab8572fde2

New changeset de90f3d06bc9 by Victor Stinner in branch 'default':
Argument Clinic: Use METH_FASTCALL for positionals
https://hg.python.org/cpython/rev/de90f3d06bc9

New changeset dea8922751a1 by Victor Stinner in branch 'default':
Run Argument Clinic: METH_VARARGS=>METH_FASTCALL
https://hg.python.org/cpython/rev/dea8922751a1

vstinner · 2017-01-17T01:37:51Z

Naoki> This patch makes AC produces more FASTCALL instead of VARARGS.

Oh, funny, I wrote the same patch :-) (almost)

When looking AC output, one downside is it produces many consts like:

static const char * const _keywords[] = {"", "", "", "", "", NULL};

Yeah, I got the same result. I tried to hack a _PyArg_ParseStack() function which doesn't take keyword argments (no kwnames), but I lost my mind in parser_init().

So I decided to simply rebase my old patches from my random CPython fastcall forks to add a simple _PyArg_ParseStack(). My implementation doesn't use the super-fast _PyArg_Parser object, but at least it allows to use METH_FASTCALL. Compared to what we had previously (METH_VARARGS), it is an enhancement :-)

I prefer to move step by step, since each commit is already big enough. It's also easier for me to maintain my forks if I push more changes upstream.

So there is still room for improvement in _PyArg_ParseStack(), but I suggest to defer further optimizations to a new issue.

python-dev · 2017-01-17T01:48:49Z

New changeset 3bf78c286daf by Victor Stinner in branch 'default':
Add _PyArg_UnpackStack() function helper
https://hg.python.org/cpython/rev/3bf78c286daf

New changeset 905e902bd47e by Victor Stinner in branch 'default':
Argument Clinic: Use METH_FASTCALL for boring positionals
https://hg.python.org/cpython/rev/905e902bd47e

New changeset 52acda52b353 by Victor Stinner in branch 'default':
Run Argument Clinic: METH_VARARGS=>METH_FASTCALL
https://hg.python.org/cpython/rev/52acda52b353

python-dev · 2017-01-17T02:25:18Z

New changeset d07fd6e6d449 by Victor Stinner in branch 'default':
Rename _PyArg_ParseStack to _PyArg_ParseStackAndKeywords
https://hg.python.org/cpython/rev/d07fd6e6d449

New changeset 01c57ef1b651 by Victor Stinner in branch 'default':
Add _PyArg_ParseStack() helper function
https://hg.python.org/cpython/rev/01c57ef1b651

New changeset 8bfec37ea86a by Victor Stinner in branch 'default':
Add _PyArg_NoStackKeywords() helper function
https://hg.python.org/cpython/rev/8bfec37ea86a

New changeset 38ab8572fde2 by Victor Stinner in branch 'default':
_PyStack_UnpackDict() now returns -1 on error
https://hg.python.org/cpython/rev/38ab8572fde2

New changeset de90f3d06bc9 by Victor Stinner in branch 'default':
Argument Clinic: Use METH_FASTCALL for positionals
https://hg.python.org/cpython/rev/de90f3d06bc9

New changeset dea8922751a1 by Victor Stinner in branch 'default':
Run Argument Clinic: METH_VARARGS=>METH_FASTCALL
https://hg.python.org/cpython/rev/dea8922751a1

serhiy-storchaka · 2017-01-17T06:24:02Z

Oh, I have wrote almost the same patch before going to sleep yesteday! ;) But the building crashed (likely due to a bug in _PyStack_UnpackDict()) and it was too late to resolve this.

I would prefer to rename "l" to "nargs" in PyArg_UnpackStack_impl and don't use upper case and namespace prefix in static functions.

vstinner · 2017-02-01T16:36:31Z

Recently, I saw complains that I push changes too fast without reviews. This issue is a good example. So let me elaborate myself a little bit on these changes.

I'm not really proud of the "Rename _PyArg_ParseStack to _PyArg_ParseStackAndKeywords" change. It should be called _PyArg_ParseStackAndKeywords() from the beginning, but when I designed FASTCALL, my plan was to only check arguments (positional and keyword arguments) in the callee, and remove checks in the caller. I expected that all functions will get positional + keyword arguments.

I didn't know that not accepting keywords but only positional arguments like in str.replace(), was a deliberate language design choice.

So yes, for functions accepting only positional arguments, _PyArg_ParseStack() + _PyArg_NoStackKeywords() makes sense.

IMHO the implementation of the new _PyArg_ParseStack() and _PyArg_NoStackKeywords() functions was simple enough to avoid a review, they reuse existing code with minimum changes.

In general for FASTCALL, I only add private functions. I consider that it's ok to push code quickly, since the private property allows us to change these functions anytime.

Again, I consider that the "Argument Clinic: Use METH_FASTCALL for positionals" change is simple enough to avoid a review, I don't think that they are many ways to use FASTCALL for functions using positional-only parameters. I mean, I only see a single way to implement it. Again, if someone sees a new way to handle such parameters, we can change it anytime, Argument Clinic and FASTCALL are stil private.

More generally, FASTCALL requires a lot of tiny changes everywhere.

GitHub will allow to get reviews on long patch series. It will be easier to handle large changes.

python-dev · 2017-02-01T16:42:57Z

New changeset 758674087b12 by Victor Stinner in branch 'default':
Issue bpo-29286: Rename private PyArg_UnpackStack_impl() to unpack_stack()
https://hg.python.org/cpython/rev/758674087b12

vstinner · 2017-02-01T16:43:58Z

Serhiy Storchaka: "Oh, I have wrote almost the same patch before going to sleep yesteday! ;) But the building crashed (likely due to a bug in _PyStack_UnpackDict()) and it was too late to resolve this."

Oh, sorry that you wrote almost the same code. Well, at least it means that we agree on the design :-)

Serhiy: "I would prefer to rename "l" to "nargs" in PyArg_UnpackStack_impl and don't use upper case and namespace prefix in static functions."

Done.

vstinner · 2017-02-01T16:45:09Z

str type still has a few methods using METH_VARARGS (slower than METH_FASTCALL):

count()
find()
index()
rfind()
rindex()
startswith()
endswith()
format()

Maybe I will propose a change later to convert these methods to Argument Clinic, but I consider that the initial issue "Currently, Argument Clinic doesn't use METH_FASTCALL for these methods, but METH_VARARGS" is now fixed, so I close the issue.

Thanks Naoki and Serhiy.

vstinner · 2017-06-09T11:24:56Z

New changeset f0ff849 by Victor Stinner in branch '3.6':
bpo-30524: Fix _PyStack_UnpackDict() (bpo-1886)
f0ff849

python-dev · 2022-04-09T06:25:34Z

New changeset 5910fd7231d34363798d2815be2f66909e638d1c by Victor Stinner in branch 'master':
Issue bpo-29286: Rename private PyArg_UnpackStack_impl() to unpack_stack()
5910fd7

vstinner added 3.7 (EOL) end of life performance Performance or resource usage labels Jan 16, 2017

vstinner closed this as completed Feb 1, 2017

ezio-melotti transferred this issue from another repository Apr 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use METH_FASTCALL in str methods #73472

Use METH_FASTCALL in str methods #73472

vstinner commented Jan 16, 2017

vstinner commented Jan 16, 2017

vstinner commented Jan 16, 2017

vstinner commented Jan 16, 2017

1st1 commented Jan 16, 2017

serhiy-storchaka commented Jan 16, 2017

vstinner commented Jan 16, 2017

methane commented Jan 16, 2017

methane commented Jan 17, 2017

vstinner commented Jan 17, 2017

python-dev mannequin commented Jan 17, 2017

vstinner commented Jan 17, 2017

python-dev mannequin commented Jan 17, 2017

python-dev mannequin commented Jan 17, 2017

serhiy-storchaka commented Jan 17, 2017

vstinner commented Feb 1, 2017

python-dev mannequin commented Feb 1, 2017

vstinner commented Feb 1, 2017

vstinner commented Feb 1, 2017

vstinner commented Jun 9, 2017

python-dev mannequin commented Apr 9, 2022

Use METH_FASTCALL in str methods #73472

Use METH_FASTCALL in str methods #73472

Comments

vstinner commented Jan 16, 2017

vstinner commented Jan 16, 2017

vstinner commented Jan 16, 2017

vstinner commented Jan 16, 2017

1st1 commented Jan 16, 2017

serhiy-storchaka commented Jan 16, 2017

vstinner commented Jan 16, 2017

methane commented Jan 16, 2017

methane commented Jan 17, 2017

vstinner commented Jan 17, 2017

python-dev mannequin commented Jan 17, 2017

vstinner commented Jan 17, 2017

python-dev mannequin commented Jan 17, 2017

python-dev mannequin commented Jan 17, 2017

serhiy-storchaka commented Jan 17, 2017

vstinner commented Feb 1, 2017

python-dev mannequin commented Feb 1, 2017

vstinner commented Feb 1, 2017

vstinner commented Feb 1, 2017

vstinner commented Jun 9, 2017

python-dev mannequin commented Apr 9, 2022