Rework CALL_FUNCTION* opcodes

@vstinner

BPO	27213
Nosy	@vstinner, @rbtcollins, @markshannon, @berkerpeksag, @serhiy-storchaka, @1st1, @serprex
Dependencies	bpo-27140: Opcode for creating dict with constant keys
Files	callfunc.patch callfunc2.patch callfunc3.patch callfunc4.patch callfunc5.patch callfunc-6.patch callfunc-7.patch callfunc-8.patch callfunc-9.patch callfunc-10.patch callfunc-11.patch callfunc-12.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/serhiy-storchaka'
closed_at = <Date 2016-09-22.16:54:56.652>
created_at = <Date 2016-06-04.07:45:33.985>
labels = ['interpreter-core', 'type-feature']
title = 'Rework CALL_FUNCTION* opcodes'
updated_at = <Date 2016-09-22.21:13:20.263>
user = 'https://github.com/serhiy-storchaka'

bugs.python.org fields:

activity = <Date 2016-09-22.21:13:20.263>
actor = 'vstinner'
assignee = 'serhiy.storchaka'
closed = True
closed_date = <Date 2016-09-22.16:54:56.652>
closer = 'serhiy.storchaka'
components = ['Interpreter Core']
creation = <Date 2016-06-04.07:45:33.985>
creator = 'serhiy.storchaka'
dependencies = ['27140']
files = ['43474', '43493', '43602', '43669', '43741', '44403', '44485', '44487', '44508', '44510', '44546', '44551']
hgrepos = ['354']
issue_num = 27213
keywords = ['patch']
message_count = 55.0
messages = ['267241', '267341', '268285', '268652', '268720', '268860', '268862', '268907', '268948', '268998', '269623', '269669', '269696', '270064', '270521', '273372', '273684', '273685', '274641', '275259', '275261', '275264', '275295', '275348', '275351', '275352', '275372', '275387', '275388', '275402', '275405', '275409', '275411', '275412', '275448', '275452', '275748', '275749', '275752', '275766', '275770', '275784', '275861', '275885', '276004', '276006', '276008', '276021', '276022', '276063', '276064', '276073', '276102', '277233', '277239']
nosy_count = 8.0
nosy_names = ['vstinner', 'rbcollins', 'Mark.Shannon', 'python-dev', 'berker.peksag', 'serhiy.storchaka', 'yselivanov', 'Demur Rumed']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue27213'
versions = ['Python 3.6']

Currently there are 4 opcodes (CALL_FUNCTION, CALL_FUNCTION_VAR, CALL_FUNCTION_KW, CALL_FUNCTION_VAR_KW) for calling a function depending of presenting the var-positional and var-keyword arguments:

    func(arg1, ..., argN, name1=kwarg1, ..., nameM=kwargM)
    func(arg1, ..., argN, *args, name1=kwarg1, ..., nameM=kwargM)
    func(arg1, ..., argN, name1=kwarg1, ..., nameM=kwargM, **kwargs)
    func(arg1, ..., argN, *args, name1=kwarg1, ..., nameM=kwargM, **kwargs)

The number of positional and keyword arguments are packed in oparg, and both numbers are limited by 255. Thus the single keyword argument makes oparg not fitting in 8 bit and requires EXTENDED_ARG. The stack contains first positional arguments, then optional args tuple, then keyword arguments, then optional kwargs dict. For every keyword argument the two values are pushed on the stack: argument name (always constant string) and argument value.

I collected a statistic about opcodes in compiled code during running Python tests [1] (maybe it is biased, but this is large and multifarious assembly, and I hope it takes some representation about average Python code). According to it about 90% of compiled function calls are calls with the fixed number of positional arguments (CALL_FUNCTION with oparg < 256), the rest 10% are calls with the fixed number of positional and keyword arguments (CALL_FUNCTION with oparg >= 256), and only about 0.5% are calls with the var-positional or var-keyword arguments.

I propose to use the different sets of opcodes that corresponds to these cases:

    func(arg1, ..., argN)
    func(arg1, ..., argN, name1=kwarg1, ..., nameM=kwargM)
    func(*args, **kwargs)

CALL_FUNCTION takes the fixed number of positional arguments. oparg is the number of arguments. The stack contains positional arguments.
CALL_FUNCTION_KW takes the fixed number of positional and keyword arguments. oparg is the number of all arguments. The stack contains values of arguments (first positional, then keyword), then a tuple of keyword names (as in proposed new opcode BUILD_CONST_KEY_MAP in bpo-27140).
CALL_FUNCTION_EX takes the variable number of positional and keyword arguments. oparg is 0. The stack contains a tuple of positional arguments and a dict of keyword arguments (they are build in the bytecode with using BUILD_TUPLE, BUILD_CONST_KEY_MAP, BUILD_LIST_UNPACK and BUILD_MAP_UNPACK_WITH_CALL). This is the most general variant, others exist just for the optimization of common cases.

Benefits:

Calling a function with keyword arguments uses less stack and less LOAD_CONST instructions.
Calling a function with keyword arguments no longer needs EXTENDED_ARG.
The number of positional and keyword arguments is no longer limited by 255 (at least not in the bytecode).
The bytecode looks simpler, oparg always is just the number of arguments taken from the stack.

This proposition was discussed on Python-Ideas [2].

[1] http://permalink.gmane.org/gmane.comp.python.ideas/39993
[2] http://comments.gmane.org/gmane.comp.python.ideas/39961

I'd like to take on creating a patch for this proposal once bpo-27140 lands

I was going to write a patch myself, but since I'm sure in your skills, I yield it to you.

Do you take this for your Demur?

I've been working on this, may have the ceval portion mostly worked out but can't test until I finish the compile portion. Haven't had time this week, will have time to focus this weekend

Attaching first iteration. Very drafty. Still need to fix test_dis; will run test suite this evening. Perf on pybench went from 16.5s to 17.5s. It was 18.3s prior to reintroducing use of fast_function. Code still needs clean up besides investigation into how to optimize

Changes not described in the original patch concept: I made CALL_FUNCTION_EX oparg carry 2 flag bits on whether there's a kwdict or a vararg tuple. Code may be simpler to only have kwdict be optional. BUILD_MAP_UNPACK_WITH_CALL was modified to only use the highest bit of the 2nd byte to track where function_pos is. Most uses of BUILD_MAP_UNPACK_WITH_CALL is f(*va,**kw) so I chose to only set the highest bit when there isn't a positional unpacking. If we pushed an empty tuple then that flag bit could be removed

I seemed to have been added to the nosy list. I guess that means that my opinions are wanted ;)

I have wanted to clean up the code around making calls for quite a while. I just haven't had time.

I agree that adding a simpler opcode for making calls with positional only args is worthwhile.
However creating the tuple and/or dict at every call site is going to lead to slow downs. It gets even worse for bound-methods, as yet another tuple will need to be created to accommodate self.

What I would suggest is a new internal calling convention which embodies the six-part form of CALL_FUNCTION_VAR_KW, that is (number-of-postionals, pointer-to-positionals, number-of-keywords, pointer-to-keywords, starargs-tuple, doublestarargs-dict).
As a first step, each of the four current CALL_FUNCTION variants could then call this function, filling in the relevant parameters.

The new function would look something like this:
PyEval_Call_Callable(
PyObject *callable,
int npos,
PyObject **positionals,
int nkws,
PyObject **keywords,
PyObject *starargs,
PyObject *dictargs) {
    if (Is_PyFunction(callable))
        return PyFunction_Call(callable, npos, ...);

if (Is_CFunction(callable))
    return CFunction_Call(callable, npos, ...);

// Otherwise general object...

// We might want PyType_Call as well, since object creation is
// another common form of call.

// Create tuple and dict from args.
return PyObject_Call(callable, tuple, dict);

}

This would mean that the logic for frame creation would be where it belongs (in the code for the Function object) and that special casing the various forms of CFunctions could be moved to where that belongs.

Given what Serhiy says about calls with just positional parameters being by far the most common, it sounds as if it would make sense to add
PyFunction_CallSimple(PyObject *callable, int npos, PyObject**args)
and
CFuntion_CallSimple(PyObject *callable, int npos, PyObject**args)
variants for the new CALL_FUNCTION opcode to use.

With those changes, it would then make sense to replace the four opcodes
CALL_FUNCTION, CALL_FUNCTION_VAR, CALL_FUNCTION_KW, CALL_FUNCTION_VAR_KW

with
CALL_FUNCTION -- As Serhiy suggests
CALL_FUNCTION_KW -- As Serhiy suggests
CALL_FUNCTION_VAR_KW -- As currently exists

Thank you Demur. I left few comments on Rietveld (this is only the quick glance).

Good news that this change allows to simplify BUILD_MAP_UNPACK_WITH_CALL. But using 15th bit means using EXTENDED_ARG. I think it would be simpler to just push an empty tuple with LOAD_CONST. The same number of opcodes, but simpler implementation and nicer disassemble output.

Mark's idea about using oparg of the general variant for specifying a number of positional arguments without packing them in a tuple looks interesting. But I don't bother about this case. According to the statistics of running CPython tests *args or *kwargs are used only in 0.4% of calls. This is hardly worth optimizing. I'm going to collect more detailed statistics for number of args and function/method distinguishing.

As about PyFunction_CallSimple() etc, see bpo-26814 and bpo-27128. _PyObject_FastCall() protocol and new calling protocol should cover the case of new CALL_FUNCTION and may be CALL_FUNCTION_KW (>99% of calls in sum).

callfunc2 fixes test_dis, addresses code review, currently implements a copy of _PyEval_EvalCodeWithName as _PyEval_EvalCodeWithName2 which changes a few lines to work with new keyword stack layout so that we can use fast_function with kwargs

CALL_FUNCTION_EX is benchmarking much slower (4x slower when using **kw), but unpatched hits similar perf when using multiple **kw. So most slowdown is due to BUILD_MAP_UNPACK_WITH_CALL being slow. So some patch which speeds up intersection check (eg optimize to not allocate intersection when disjoint) should greatly diminish the perf loss on this simpler implementation. I'll open a separate issue for this

bpo-27358 is too complex, more complex than this issue. But I think the simple fix the regression in case of the single **kw is checking wherever the sum dict is empty.

```
           if (with_call) {
```

+ if (with_call && PyDict_Size(sum)) {

Added comments on Rietveld.

callfunc3 addresses most feedback. Doesn't address _PyEval_EvalCodeWithName2 code bloat, & I disagree with mentioning BUILD_MAP_UNPACK_WITH_CALL change in magic number update as the ABI of BUILD_MAP_UNPACK_WITH_CALL is unchanged. ie if we were to implement bpo-27358 after this there would be no need to bump the magic number

One thing which looks odd to me is the INCREF/DECREF calls on function objects surrounding calls. Especially odd is CALL_FUNCTION_EX where we finish with two calls to Py_DECREF(func). It shows up in IMPORT_NAME too

The ABI of BUILD_MAP_UNPACK_WITH_CALL is changed. oparg=514 meant merging 2 dicts, but with the patch it will mean merging 514 dicts.

The INCREF/DECREF calls on function objects surrounding calls are not needed, because the refcount was increased when the function pushed on the stack. They were needed if use PyMethod optimization, since PyMethod_GET_FUNCTION() returns borrowed link.

Seems you have missed my comments on compile.c.

Added more comments on Rietveld.

What about CALL_FUNCTION_EX benchmarking? Did the optimization help?

New changeset 51b635e81958 by Serhiy Storchaka in branch 'default':
Issue bpo-27213: Fixed different issues with reworked CALL_FUNCTION* opcodes.
https://hg.python.org/cpython/rev/51b635e81958

See http://bugs.python.org/issue28086 - this introduced a regression in the test suite.

I have committed the patch before beta 1 because it change bytecode generating and interpreting. The bytecode generated before the patch is compatible with the patched interpreter, but the bytecode generated by patched compiler can be not compatible with unpatched interpreter. After releasing beta 1 this would require changing bytecode magic number.

The change 51b635e81958 introduced memory leaks. Example:

$ ./python -m test -R 3:3 test_extcall
(...)
test_extcall leaked [102, 102, 102] references, sum=306
test_extcall leaked [41, 41, 41] memory blocks, sum=123

I'm analyzing the change to try to identify leaks.

New changeset f217419d08f0 by Victor Stinner in branch 'default':
Issue bpo-27213: Fix reference leaks
https://hg.python.org/cpython/rev/f217419d08f0

New changeset f860b7a775c5 by Victor Stinner in branch 'default':
ssue bpo-27213: Reintroduce checks in _PyStack_AsDict()
https://hg.python.org/cpython/rev/f860b7a775c5

New changeset e372c0ad32ce by Victor Stinner in branch 'default':
Revert change f860b7a775c5
https://hg.python.org/cpython/rev/e372c0ad32ce

New changeset 2558bc4a4ebf by Victor Stinner in branch 'default':
Document kwnames in _PyObject_FastCallKeywords() and _PyStack_AsDict()
https://hg.python.org/cpython/rev/2558bc4a4ebf

After releasing beta 1 this would require changing bytecode magic number.

Why don't we change the magic number before b1 (if your bytecode change is already committed)?

Why don't we change the magic number before b1 (if your bytecode change is already committed)?

The magic number changed many times between Python 3.5 and the future Python 3.6 beta 1. Maybe we skipped increasing this number for one bytecode change, but it should only impact the few developers who worked on the development version. In this case: just remove all ".pyc" files and it should be fine ;-)

In short: I think it's fine, don't do anything else ;-)

Objects/methodobject.c: In function ‘_PyCFunction_FastCallKeywords’:
Objects/methodobject.c:281:24: warning: ‘nkwargs’ may be used uninitialized in this function [-Wmaybe-uninitialized]
assert((nargs == 0 && nkwargs == 0) || stack != NULL);
^

Thanks for the report, warning fixed in the issue bpo-28105.

I think that's all with this issue.

I think that's all with this issue.

Thanks Demur and Serhiy for your great work!

Navigation Menu

Repository navigation

Rework CALL_FUNCTION* opcodes #71400

30 remaining items

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Rework CALL_FUNCTION* opcodes #71400

Description

Activity

serhiy-storchaka commented on Jun 4, 2016

serprex commented on Jun 4, 2016

serhiy-storchaka commented on Jun 11, 2016

serhiy-storchaka commented on Jun 16, 2016

serprex commented on Jun 17, 2016

serprex commented on Jun 19, 2016

markshannon commented on Jun 19, 2016

serhiy-storchaka commented on Jun 20, 2016

serprex commented on Jun 20, 2016

serhiy-storchaka commented on Jun 21, 2016

serhiy-storchaka commented on Jun 30, 2016

serprex commented on Jul 1, 2016

serhiy-storchaka commented on Jul 1, 2016

30 remaining items

python-dev commented on Sep 11, 2016

rbtcollins commented on Sep 11, 2016

serhiy-storchaka commented on Sep 12, 2016

vstinner commented on Sep 12, 2016

python-dev commented on Sep 12, 2016

python-dev commented on Sep 12, 2016

python-dev commented on Sep 12, 2016

1st1 commented on Sep 12, 2016

vstinner commented on Sep 12, 2016

serhiy-storchaka commented on Sep 12, 2016

vstinner commented on Sep 12, 2016

serhiy-storchaka commented on Sep 22, 2016

vstinner commented on Sep 22, 2016

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions