Proposed patch renames METH_FASTCALL to METH_FASTCALL|METH_KEYWORDS and makes bare METH_FASTCALL be used for functions with positional-only parameters. This eliminates small cost that these functions pay for handling empty keywords: calling _PyStack_UnpackDict() and _PyArg_NoStackKeywords(), passing kwnames. This also can slightly reduce stack consumption.

> Proposed patch renames METH_FASTCALL to METH_FASTCALL|METH_KEYWORDS and makes bare METH_FASTCALL be used for functions with positional-only parameters.

While I tried to keep everything related to FASTCALL private, it seems like Cython uses some FASTCALL features. I don't know which ones exactly. Well, if only one project in the world uses FASTCALL, we can help them to support such backward incompatible change ;-)


> This eliminates small cost that these functions pay for handling empty keywords: calling _PyStack_UnpackDict() and _PyArg_NoStackKeywords(), passing kwnames.

My idea when I designed FASTCALL was to move code to parse arguments in the function body rather than in _PyCFunction_FastCallKeywords(), and to have a single calling function METH_FASTCALL, rather than two (METH_FASTCALL and METH_FASTCALL|METH_KEYWORDS).

The long term plan is also to support passing arguments by keyword in more functions. IMHO many functions don't accept keywords for technical reasons, but once we converted a module, function or type to Argument Clinic, it becomes trivial to accept keywords. If most functions accept keywords, I'm not sure that having a special case for positional-only is still worth it. But this plan was before I had discussions on supporting keywords in unicode methods. In fact, it's deliberate to not accept keywords in many functions or methods.

Well, when I see your patch, I see that it removes a lot of code. So it's likely to be a good idea :-)


> This also can slightly reduce stack consumption.

You mean the removal of the "PyObject *kwnames" for METH_FASTCALL (positional arguments only)? Do you have an idea of the stack usage? Try maybe testcapi_stacksize.patch of the issue #28870? It would help to take a decision on this change.

We can avoid breaking backward compatibility and introduce new call method METH_FASTCALL_NO_KEYWORDS. But combining existing flags looks better to me. FASTCALL is not a part of stable ABI.

I still didn't do any benchmarking or stack usage measurements.

> I still didn't do any benchmarking or stack usage measurements.

I'm running benchmarks on the speed-python server.

Adding Stefan Behnel, perhaps Cython doesn't need backwards compatibility.

I measured the stack consumption, it's not better. But I created the issue #29465 "Add _PyObject_FastCall() to reduce stack consumption" which would allow to reduce the stack consumption with this patch.

> We can avoid breaking backward compatibility and introduce new call method METH_FASTCALL_NO_KEYWORDS. But combining existing flags looks better to me. FASTCALL is not a part of stable ABI.

If we decide that having two FASTCALL calling convention, I prefer what you proposed: METH_FASTCALL (pos only) and METH_FASTCALL|METH_KEYWORDS (pos+kw). As you wrote, I like reusing the existing METH_KEYWORDS flag, it reduces the surprises if someone ports existing code using METH_VARARGS and METH_VARARGS|METH_KEYWORDS.

Hum, benchmark results don't seem good. There is probably a performance bug somewhere. I should investigate further to analyze these results. M aybe combined with the issue #29465, results will be better.

haypo@speed-python$ python3 -m perf compare_to ~/benchmarks/2017-02-06_07-15-default-e06af4027546.json fastcall-no-keywords_ref_e06af4027546.json -G --min-speed=5

Slower (19):
- unpickle_list: 6.36 us +- 0.11 us -> 8.08 us +- 0.14 us: 1.27x slower (+27%)
- pickle_list: 7.53 us +- 0.41 us -> 8.62 us +- 0.49 us: 1.14x slower (+14%)
- crypto_pyaes: 199 ms +- 2 ms -> 226 ms +- 2 ms: 1.13x slower (+13%)
- pickle: 22.1 us +- 0.3 us -> 24.9 us +- 0.3 us: 1.12x slower (+12%)
- nbody: 233 ms +- 2 ms -> 260 ms +- 2 ms: 1.12x slower (+12%)
- xml_etree_iterparse: 179 ms +- 4 ms -> 198 ms +- 5 ms: 1.11x slower (+11%)
- telco: 14.7 ms +- 0.3 ms -> 16.3 ms +- 0.5 ms: 1.11x slower (+11%)
- pickle_dict: 56.6 us +- 4.3 us -> 62.7 us +- 4.7 us: 1.11x slower (+11%)
- pidigits: 291 ms +- 1 ms -> 319 ms +- 1 ms: 1.10x slower (+10%)
- scimark_fft: 662 ms +- 10 ms -> 717 ms +- 8 ms: 1.08x slower (+8%)
- scimark_monte_carlo: 207 ms +- 4 ms -> 224 ms +- 6 ms: 1.08x slower (+8%)
- regex_v8: 43.7 ms +- 0.6 ms -> 47.0 ms +- 0.3 ms: 1.08x slower (+8%)
- float: 238 ms +- 3 ms -> 254 ms +- 4 ms: 1.07x slower (+7%)
- xml_etree_parse: 242 ms +- 5 ms -> 257 ms +- 9 ms: 1.06x slower (+6%)
- raytrace: 1.04 sec +- 0.01 sec -> 1.11 sec +- 0.01 sec: 1.06x slower (+6%)
- unpickle: 30.0 us +- 0.3 us -> 31.8 us +- 0.3 us: 1.06x slower (+6%)
- go: 493 ms +- 7 ms -> 520 ms +- 6 ms: 1.05x slower (+5%)
- scimark_sparse_mat_mult: 8.24 ms +- 0.14 ms -> 8.69 ms +- 0.14 ms: 1.05x slower (+5%)
- chaos: 234 ms +- 2 ms -> 246 ms +- 4 ms: 1.05x slower (+5%)

Faster (2):
- chameleon: 21.9 ms +- 0.2 ms -> 20.7 ms +- 0.3 ms: 1.06x faster (-5%)
- sympy_expand: 949 ms +- 12 ms -> 899 ms +- 11 ms: 1.06x faster (-5%)

Benchmark hidden because not significant (43): (...)

Thanks for asking. Cython doesn't use METH_FASTCALL yet, so this doesn't introduce any problems.

Generally speaking, if Cython generated user code stops working with a new CPython version, we expect people to regenerate their code with the newest Cython version to fix it, so this kind of internal incompatibility is usually reparable by adapting Cython appropriately.

There was a bug in previous patch. The signature of generated by Argument Clinic functions was not changed. Updated patch fixes this bug and also fixes the use of fast call in deque methods.

Ok, so I looked again at your change: fastcall-no-keywords-2.patch LGTM,
I like the idea!

Since your patch is huge, I expect that it will be a pain to rebase it. I
suggest you to push it as soon as possible, to avoid conflicts. I will rework
my issue #29465 patch once this issue is done.


According to the number of modified functions and files, not accepting keyword
arguments is very common, and so it is annoying to have to call
_PyArg_NoStackKeywords() in each function. Avoiding the "PyObject *kwnames" can
also reduces the stack consumption per call, especially in long call stacks.

At least my assumption that almost no function will accept only positional
arguments is plain wrong :-) In practice, it's more the opposite!

I checked quickly fastcall-no-keywords-2.patch stack usage and performance.
It seems non significant, but I'm not surprised, it isn't really the purpose
of the change. IMHO the purpose of the change is more to simplify the code,
avoid to duplicate _PyArg_NoStackKeywords() everywhere, and only check
keyword arguments at one place.

For stack consumption and performances, I got good results on my issue #29465
which adds a new _PyObject_FastCall() function. IMHO this function fits well
with your new METH_FASTCALL (no keyword argument)! If we combine both changes,
we can get something very good ;-)

@Stefan Behnel: Thank you for you reply. So the backward compatibility was a
fake issue, and we are free to modify METH_FASTCALL. Anyway, *if* someone uses
METH_FASTCALL, IMHO it will be easy to update his/her code and add #ifdef on
the Python version if needed. Again, as Serhiy wrote, METH_FASTCALL is out of
the stable ABI, so we are safe on our warranties.

The major part of the patch is generated by Argument Clinic. It is not a pain to rebase it. I prefer to delay pushing the patch until we prove its usefulness, because the cost of this change is not zero. Is it a stopper for issue29465?

Serhiy Storchaka: "I prefer to delay pushing the patch until we prove its usefulness, because the cost of this change is not zero. Is it a stopper for issue29465?"

Ok. No, it's not a blocker for my issue #29465.

About usefulness, I'm curious of the performance impact on this patch on top of the issue #29465. I tried to run a benchmark, but my tooling only works well with a single patch, not with two patches, and one based on the other. Moreover, our patches are in conflict.

So it seems like the issue #29465 makes Python faster and uses less stack memory, I suggest to first focus on that one, and then rebase your patch on top of that. What do you think?

> Proposed patch renames METH_FASTCALL to METH_FASTCALL|METH_KEYWORDS
> and makes bare METH_FASTCALL be used for functions with 
> positional-only parameters.


+1

I looked up this change again and was surprised that it still wasn't applied. It feels to me that it makes sense already for reasons of consistency. Any time frame for changing it? I'd like to use METH_FASTCALL in Cython in a future-proof way.

> Proposed patch renames METH_FASTCALL to METH_FASTCALL|METH_KEYWORDS
> and makes bare METH_FASTCALL be used for functions with 
> positional-only parameters. This eliminates small cost that 
> these functions pay for handling empty keywords: calling 
> _PyStack_UnpackDict() and _PyArg_NoStackKeywords(), 
> passing kwnames. This also can slightly reduce stack 
> consumption.

+1 for all the reasons listed.  These are very reasonable specializations.  The empty keyword checks are really irritating for fine-grained functions.

> I'd like to use METH_FASTCALL in Cython in a future-proof way.

Also +1.  Can we consider the API frozen after this issue or do we have to wait for #29465 (or others)?

Actually these arguments are pretty weak. The sole argument of consistency is weak itself. _PyArg_NoStackKeywords() is pretty cheap since implementing it as a macro. This change just moves the check from the implementation of functions to the calling place. Virtually all affected functions (320 vs 7) are generated by Argument Clinic, so this doesn't simplify the maintenance much. The only runtime effect is possible saving a register or few bytes on the stack and few CPU tacts for passing the kwnames argument (always NULL). But this depends on the compiler.

I don't consider this API frozen still. I have other idea for passing keyword arguments and want to experiment with it.

Victor, could you please rerun benchmarks and check whether this change cause performance degradation?

For third party projects who want to use FASTCALL the functions are not generated by AC.  I didn't understand the reasoning why the consistency argument is weak.

Here are benchmark results. Sorry, but I'm not really convinced that this specialization is worth it.

The change adds yet another calling convention where we already have METH_NOARG, METH_VARARGS, METH_O, METH_NOARG | METH_KEYWORDS, METH_FASTCALL...

I'm ok to add a new calling convention but only if it's faster on more benchmarks or if it uses much less memory. It doesn't seem to be the case with the current change.


Differences of at least 5%:

haypo@speed-python$ python3 -m perf compare_to /home/haypo/json/2017-06-09_08-18-master-ef8320cf6f09.json.gz ~/json/patch/2017-06-09_08-18-master-ef8320cf6f09-patch-1955.json.gz -G --min-speed=5 --table

+-------------------------+--------------------------------------+-------------------------------------------------+
| Benchmark               | 2017-06-09_08-18-master-ef8320cf6f09 | 2017-06-09_08-18-master-ef8320cf6f09-patch-1955 |
+=========================+======================================+=================================================+
| spectral_norm           | 283 ms                               | 263 ms: 1.08x faster (-7%)                      |
+-------------------------+--------------------------------------+-------------------------------------------------+
| scimark_lu              | 294 ms                               | 314 ms: 1.07x slower (+7%)                      |
+-------------------------+--------------------------------------+-------------------------------------------------+
| scimark_sparse_mat_mult | 8.15 ms                              | 9.12 ms: 1.12x slower (+12%)                    |
+-------------------------+--------------------------------------+-------------------------------------------------+


Differences of at least 2%:

haypo@speed-python$ python3 -m perf compare_to /home/haypo/json/2017-06-09_08-18-master-ef8320cf6f09.json.gz ~/json/patch/2017-06-09_08-18-master-ef8320cf6f09-patch-1955.json.gz -G --min-speed=2 --table

+-------------------------+--------------------------------------+-------------------------------------------------+
| Benchmark               | 2017-06-09_08-18-master-ef8320cf6f09 | 2017-06-09_08-18-master-ef8320cf6f09-patch-1955 |
+=========================+======================================+=================================================+
| spectral_norm           | 283 ms                               | 263 ms: 1.08x faster (-7%)                      |
+-------------------------+--------------------------------------+-------------------------------------------------+
| genshi_text             | 73.1 ms                              | 70.5 ms: 1.04x faster (-3%)                     |
+-------------------------+--------------------------------------+-------------------------------------------------+
| scimark_monte_carlo     | 209 ms                               | 201 ms: 1.04x faster (-3%)                      |
+-------------------------+--------------------------------------+-------------------------------------------------+
| raytrace                | 1.05 sec                             | 1.02 sec: 1.02x faster (-2%)                    |
+-------------------------+--------------------------------------+-------------------------------------------------+
| regex_v8                | 40.3 ms                              | 41.4 ms: 1.03x slower (+3%)                     |
+-------------------------+--------------------------------------+-------------------------------------------------+
| json_dumps              | 26.2 ms                              | 27.0 ms: 1.03x slower (+3%)                     |
+-------------------------+--------------------------------------+-------------------------------------------------+
| float                   | 207 ms                               | 215 ms: 1.04x slower (+4%)                      |
+-------------------------+--------------------------------------+-------------------------------------------------+
| crypto_pyaes            | 199 ms                               | 207 ms: 1.04x slower (+4%)                      |
+-------------------------+--------------------------------------+-------------------------------------------------+
| scimark_fft             | 644 ms                               | 675 ms: 1.05x slower (+5%)                      |
+-------------------------+--------------------------------------+-------------------------------------------------+
| scimark_lu              | 294 ms                               | 314 ms: 1.07x slower (+7%)                      |
+-------------------------+--------------------------------------+-------------------------------------------------+
| scimark_sparse_mat_mult | 8.15 ms                              | 9.12 ms: 1.12x slower (+12%)                    |
+-------------------------+--------------------------------------+-------------------------------------------------+

I do not see this as a matter of performance but as a matter of usability. Basically, CPython could do just fine with just a single catch-all calling convention that packs all pos/kw arguments into C arguments and passes them over, leaving it entirely to the callee to handle them. Why does it not do that? Because it's cumbersome for the callee to analyse what kind of arguments it received and how to handle them.

A surprisingly large number of functions take only a single argument, that's why METH_O exists. Many functions take no keyword arguments, that's why VARARGS and KEYWORDS are separate options. The same should apply to FASTCALL. Also with that, many implementors will not want to care about keyword arguments and would thus appreciate it if they didn't have to. Forcing them to test for keyword arguments and raising the correct error for it (with the correct and consistent message) seems an entirely unnecessary imposition.

> Also with that, many implementors will not want to care about keyword arguments and would thus appreciate it if they didn't have to. Forcing them to test for keyword arguments and raising the correct error for it (with the correct and consistent message) seems an entirely unnecessary imposition.

Ah, I took the habit of using Argument Clinic, so I don't have to both to thing anymore :-) But yes, writing manually the PyArg_XXX() code is boring and error-prone, I agree.

Can the PR be applied then? It looks good to me.

> Can the PR be applied then? It looks good to me.

Go for it Serhiy. Even if it isn't faster today, we might find more optimizations later, at least in term of C stack consumption.

Moreover, I understand perfectly the cost of having to parse arguments. Checking for keywords in the caller seems reasonable.

This change breaks the backward compatibility with Python 3.6, right? I mean, code using METH_FASTCALL. I guess that only Cython extensions use it in the wild, and I expect that Cython extensions don't use the stable ABI but are recompiled for each 3.x release, so it should be fine in practice.

Oh, it seems like this change should help to use FASTCALL in the _decimal module: issue #29301. Stefan doesn't want to use Argument Clinic.

New changeset 6969eaf4682beb01bc95eeb14f5ce6c01312e297 by Serhiy Storchaka in branch 'master':
bpo-29464: Rename METH_FASTCALL to METH_FASTCALL|METH_KEYWORDS and make (#1955)
https://github.com/python/cpython/commit/6969eaf4682beb01bc95eeb14f5ce6c01312e297

New changeset 8207c17486baece8ed0ac42d9f8d69ecec4ba7e4 by Victor Stinner in branch 'master':
Revert "bpo-30822: Fix testing of datetime module." (#2588)
https://github.com/python/cpython/commit/8207c17486baece8ed0ac42d9f8d69ecec4ba7e4

Josay noticed that _PyArg_NoStackKeywords() is not used anymore except for one place (_hashopenssl.c).

Seems this is my oversign. Generated constructors in _hashopenssl.c use the METH_FASTCALL | METH_KEYWORDS calling method, but check that no keyword arguments are passed. They can now just use the METH_FASTCALL calling method. And _PyArg_NoStackKeywords() can be excluded from public header.

Let's remove this function which became useless.

Removed _PyArg_NoStackKeywords() (7e60192fe0dfd763b0d458cf0898ba4f7ac7d81a).

Hi, I observed an error while trying to install numpy after 6969eaf4682beb01bc95eeb14f5ce6c01312e297.

gcc: numpy/random/mtrand/mtrand.c
    numpy/random/mtrand/mtrand.c: In function ‘__Pyx_PyCFunction_FastCall’:
    numpy/random/mtrand/mtrand.c:44374:5: error: too many arguments to function ‘(struct PyObject * (*)(struct PyObject *, struct PyObject **, Py_ssize_t))meth’
         return (*((__Pyx_PyCFunctionFast)meth)) (self, args, nargs, NULL);
         ^

Is this a bug in Python or does it need to be fixed in numpy?

I think you need to update Cython to a version that supports new FASTCALL call convention in 3.7. If the support of FASTCALL in Cython is optional try to disable it.

Thanks, I'm not sure what that means exactly but I added the note to https://github.com/numpy/numpy/issues/9391. Perhaps a note in the Python release notes is warranted?

For future reference, this change is supported by Cython 0.26 (which is currently close to release).

Great!

Are all uses of internal CPython details optional? I would disable them by default for alpha versions of CPython.

> Are all uses of internal CPython details optional?

Well, what classifies as a "CPython detail" sometimes just becomes clear when other implementations don't have it. ;-)

But yes, the C code that Cython generates selects alternative implementations based on some more or less generic C defines, e.g. CYTHON_COMPILING_IN_CPYTHON or CYTHON_USE_PYLONG_INTERNALS or CYTHON_ASSUME_SAFE_MACROS. We enable them based on the Python implementation and even users can disable them at need. Well, and then there are obviously tons of PY_VERSION_HEX specific special cases, such as this one.


> I would disable them by default for alpha versions of CPython.

I don't see why. We track the CPython development via travis-CI and alpha versions make us aware of changes that we need to incorporate (or sometimes fight against ;-) ). These internals very rarely change across CPython releases (e.g. the PyLong type didn't really change since 3.0/2.7.0), and this one is really an exception. Excluding alpha versions would probably reduce our response time to these changes.

When I worked on FASTCALL, I wasn't sure that the whole project was worth
it. Now I am more confident that it makes function calls faster:
https://haypo.github.io/fastcall-microbenchmarks.html

Stefan, Serhiy: would it be a good idea to make _PyObject_FastCall() and
_PyObject_FastCallKeywords(), or also _PyObject_FastCallDict(), public? I
expect complains from PyPy who will have to support METH_FASTCALL as well
:-)