[issue27213] Rework CALL_FUNCTION* opcodes

[STINNER Victor]

STINNER Victor added the comment:

> I think that's all with this issue.

Thanks Demur and Serhiy for your great work!

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27213] Rework CALL_FUNCTION* opcodes

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

I think that's all with this issue.

--
resolution:  -> fixed
stage: patch review -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27213] Rework CALL_FUNCTION* opcodes

[STINNER Victor]

STINNER Victor added the comment:

Thanks for the report, warning fixed in the issue #28105.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27213] Rework CALL_FUNCTION* opcodes

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

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);
^

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27213] Rework CALL_FUNCTION* opcodes

[STINNER Victor]

STINNER Victor added the comment:

> 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 ;-)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27213] Rework CALL_FUNCTION* opcodes

[Yury Selivanov]

Yury Selivanov added the comment:

>  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)?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27213] Rework CALL_FUNCTION* opcodes

[Roundup Robot]

Roundup Robot added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27213] Rework CALL_FUNCTION* opcodes

[Roundup Robot]

Roundup Robot added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27213] Rework CALL_FUNCTION* opcodes

[Roundup Robot]

Roundup Robot added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27213] Rework CALL_FUNCTION* opcodes

[STINNER Victor]

STINNER Victor added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27213] Rework CALL_FUNCTION* opcodes

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27213] Rework CALL_FUNCTION* opcodes

[Robert Collins]

Robert Collins added the comment:

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

--
nosy: +rbcollins

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27213] Rework CALL_FUNCTION* opcodes

[Roundup Robot]

Roundup Robot added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27213] Rework CALL_FUNCTION* opcodes

[Serhiy Storchaka]

Changes by Serhiy Storchaka :


Removed file: http://bugs.python.org/file44557/IMG_20160830_174452.jpg

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27213] Rework CALL_FUNCTION* opcodes

[Влад Ключкей]

Changes by Влад Ключкей :


--
hgrepos: +354
Added file: http://bugs.python.org/file44557/IMG_20160830_174452.jpg

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27213] Rework CALL_FUNCTION* opcodes

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

Please attach a reproducer Berker. I still can't reproduce crashes. Maybe it 
depends on platform or installed third-party module or Python command line 
options.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27213] Rework CALL_FUNCTION* opcodes

[Berker Peksag]

Berker Peksag added the comment:

Thanks, Serhiy.

test_recursionlimit_recovery in test_sys passed, but now I'm getting the 
following failure:

FAIL: test_recursionlimit_fatalerror (test.test_sys.SysModuleTest)

I modified test_recursionlimit_fatalerror to use subTest() and it looks like 
the test passed when sys.setrecursionlimit(50). See i=1000 in the following 
output:

==
FAIL: test_recursionlimit_fatalerror (test.test_sys.SysModuleTest) (i=1000)
--
Traceback (most recent call last):
  File "/home/berker/hacking/cpython/default/Lib/test/test_sys.py", line 283, 
in test_recursionlimit_fatalerror
err)
AssertionError: b'Fatal Python error: Cannot recover from stack overflow' not 
found in b''

--

test_runpy still segfaults:

test_main_recursion_error (test.test_runpy.RunPathTestCase) ... Fatal Python 
error: Segmentation fault

I wrote a quick and dirty reproducer for this. I can attach here if you think 
it would be useful for debugging.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27213] Rework CALL_FUNCTION* opcodes

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

> python: Objects/abstract.c:2385: _PyStack_AsDict: Assertion
> `PyDict_GetItem(kwdict, key) != ((void *)0)' failed. Aborted (core dumped)
> generate-posix-vars failed
> make: *** [pybuilddir.txt] Error 1

Ah, it's my fault. I didn't test the latest patch with debug build. Here should 
be == instead of !=. Could you please test updated patch? Unfortunately I can's 
reproduce crashes in test_sys and test_runpy (on 32-bit Linux).

--
Added file: http://bugs.python.org/file44551/callfunc-12.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27213] Rework CALL_FUNCTION* opcodes

[Berker Peksag]

Berker Peksag added the comment:

> [...] please try a "make distclean" and make sure that all .pyc files are 
> removed, and then recompile Python.

I forgot to mention that in my earlier comment. I usually run "make distclean" 
before "./configure --with-pydebug; make -s -j" so unfortunately running "make 
distclean" didn't help.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27213] Rework CALL_FUNCTION* opcodes

[STINNER Victor]

STINNER Victor added the comment:

Author: Berker Peksag (berker.peksag) * (Python committer)  Date: 
2016-09-11 08:55

Berker: "I'm getting segfaults with a77756e480c2 (thanks to 'hg bisect') on 
Ubuntu 12.04 (...)
test_recursionlimit_recovery (...) test_main_recursion_error (...)"

We should check if the change counts correctly the number of frames.

In callfunc-10.patch, Serhiy modified a counter related to recursion 
limit/number of frames to fix test_traceback (issue #28050). He wrote on the 
review:

"Seems the number 50 is arbitrary. Now we should increase it. Why? This issue 
needs further investigation. Maybe this is a sign of regression."

To me, it's suspicious that we have to modify an unit test. Changing the opcode 
or internals must not have an impact on unit tests on recursion error.

Note: It might also be related to my FASTCALL changes, but you wrote that "hg 
bisect" pointed you to this change.


Berker: "I'm also getting the following error with Serhiy's latest patch: 
python: Objects/abstract.c:2385: _PyStack_AsDict: Assertion 
`PyDict_GetItem(kwdict, key) != ((void *)0)' failed."

This one might be related to the compact dict change.


Note: Just to make be sure that it is not a local issue, please try a "make 
distclean" and make sure that all .pyc files are removed, and then recompile 
Python.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27213] Rework CALL_FUNCTION* opcodes

[Berker Peksag]

Berker Peksag added the comment:

I'm getting segfaults with a77756e480c2 (thanks to 'hg bisect') on Ubuntu 
12.04. I've checked buildbots, but they seem happy.

In test_sys:

test_recursionlimit_recovery (test.test_sys.SysModuleTest) ... Fatal Python 
error: Segmentation fault

In test_runpy:

test_main_recursion_error (test.test_runpy.RunPathTestCase) ... Fatal Python 
error: Segmentation fault

I'm also getting the following error with Serhiy's latest patch:

python: Objects/abstract.c:2385: _PyStack_AsDict: Assertion 
`PyDict_GetItem(kwdict, key) != ((void *)0)' failed.
Aborted (core dumped)
generate-posix-vars failed
make: *** [pybuilddir.txt] Error 1

--
nosy: +berker.peksag

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27213] Rework CALL_FUNCTION* opcodes

[Serhiy Storchaka]

Changes by Serhiy Storchaka :


Added file: http://bugs.python.org/file44546/callfunc-11.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27213] Rework CALL_FUNCTION* opcodes

[STINNER Victor]

STINNER Victor added the comment:

> Here is rebased patch.

callfunc-10.patch looks like an even better enhancement compared to Python 3.5, 
nice work. Would you be ok to wait after the beta1 release? The change doesn't 
seem to change the bytecode too much, nor the C API, whereas I'm trying to push 
a few other changes for FASTCALL.

Moreover, I didn't understand fully the change, I left some questions.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27213] Rework CALL_FUNCTION* opcodes

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

Here is rebased patch.

--
Added file: http://bugs.python.org/file44510/callfunc-10.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27213] Rework CALL_FUNCTION* opcodes

[STINNER Victor]

STINNER Victor added the comment:

I just pushed my patch for the issue #27830 (add _PyObject_FastCallKeywords()). 
I'm trying to even push the first parts of METH_FASTCALL (issue #27810).

Could you please try to rebase callfunc-9.patch on top of the development 
branch? The code is moving s fast this week :-/

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27213] Rework CALL_FUNCTION* opcodes

[Roundup Robot]

Roundup Robot added the comment:

New changeset 854b08acca97 by Victor Stinner in branch 'default':
Issue #27213: document changes in Misc/NEWS
https://hg.python.org/cpython/rev/854b08acca97

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27213] Rework CALL_FUNCTION* opcodes

[STINNER Victor]

STINNER Victor added the comment:

I'm really sorry Serhiy that I didn't give you more time to finish the review 
and push yourself the change :-( Feel free to enhance the code, I don't expect 
major changes.

For test_traceback, I opened the issue #28050: if you already know how to fix 
it, please go ahead ;-)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27213] Rework CALL_FUNCTION* opcodes

[Roundup Robot]

Roundup Robot added the comment:

New changeset a77756e480c2 by Victor Stinner in branch 'default':
Rework CALL_FUNCTION* opcodes
https://hg.python.org/cpython/rev/a77756e480c2

--
nosy: +python-dev

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27213] Rework CALL_FUNCTION* opcodes

[STINNER Victor]

STINNER Victor added the comment:

> I just fixed test_extcall and test_traceback, and going to get rid of the  
> duplication in _PyEval_EvalCodeWithName. I need maybe a hour or two.

What do you mean? I already fixed test_extcall and code duplication?

"I fixed test_extcall and removed _PyEval_EvalCodeWithName2(): 
callfunc-8.patch."

I proposed to skip the failing test in test_traceback and fix it after the beta 
release, I don't think that it's a major issue, it can be fixed later.

Can you please push callfunc-8.patch right now (skipping failing tests in 
test_traceback).

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27213] Rework CALL_FUNCTION* opcodes

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

I just fixed test_extcall and test_traceback, and going to get rid of the 
duplication in _PyEval_EvalCodeWithName. I need maybe a hour or two.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27213] Rework CALL_FUNCTION* opcodes

[STINNER Victor]

STINNER Victor added the comment:

> Please wait. I just have sat down at my computer.

Can you please give me a status and/or an ETA? This change is blocking me for 2 
more changes.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27213] Rework CALL_FUNCTION* opcodes

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

Please wait. I just have sat down at my computer.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27213] Rework CALL_FUNCTION* opcodes

[STINNER Victor]

STINNER Victor added the comment:

> Please wait. I just have sat down at my computer.

Oh, you are here :-) Go ahead!

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27213] Rework CALL_FUNCTION* opcodes

[STINNER Victor]

STINNER Victor added the comment:

> I'm going to push the patch today.

I'm going to push the patch in ~10 min since I don't see it pushed yet.

> But changes to test_extcall look as a regression to me.

I agree that it looks like a regression, but it might be tricky to
retrieve the function name from the stack. I'm not even sure that the
function name is always already on the stack.

Again, I have to rush before of the feature freeze, but I hope that we
will find a smart solution for this error message.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27213] Rework CALL_FUNCTION* opcodes

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

I'm going to push the patch today. But changes to test_extcall look as a 
regression to me.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27213] Rework CALL_FUNCTION* opcodes

[Serhiy Storchaka]

Changes by Serhiy Storchaka :


--
assignee:  -> serhiy.storchaka

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27213] Rework CALL_FUNCTION* opcodes

[STINNER Victor]

STINNER Victor added the comment:

> Known issues:
> * test_traceback and test_extcall fail
> * _PyEval_EvalCodeWithName2() should be merged into 
> _PyEval_EvalCodeWithName() to factorize the code

I fixed test_extcall and removed _PyEval_EvalCodeWithName2(): callfunc-8.patch.

I don't know how to fix test_traceback.

@Serhiy: I really need the patch tomorrow. Would you mind to push it? If you 
are unable to fix test_traceback, just skip the test with a FIXME and open a 
new issue (it should be easy to fix it later).

I really want to finish METH_FASTCALL, issue #27810, before the first beta next 
monday and basically tomorrow is for me the last time when I can still push 
things.

If we really find a major design issue, you still have 3 to 4 months to revert 
the change. If we find a minor design flaw or another optimization, again, we 
will still have time to enhance the code during the beta stage. The most 
important part is to push the major change to reduce changes after the first 
beta.

--
Added file: http://bugs.python.org/file44487/callfunc-8.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27213] Rework CALL_FUNCTION* opcodes

[Yury Selivanov]

Yury Selivanov added the comment:

Serhiy, Victor and I want this to be merged tomorrow morning PST to land in 3.6 
before the freeze.  Would you be able to merge this yourself?  If not, we'll do 
it ourselves in the morning.

--
nosy: +yselivanov

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27213] Rework CALL_FUNCTION* opcodes

[STINNER Victor]

STINNER Victor added the comment:

Rebased change.

--
Added file: http://bugs.python.org/file44485/callfunc-7.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27213] Rework CALL_FUNCTION* opcodes

[STINNER Victor]

STINNER Victor added the comment:

I uploaded callfunc-6.patch which is supposed to be a rebased version of 
callfunc5.patch. I made tiny coding style changes.

Python/ceval.c changed a lot since 1 month 1/2, so I'm not 100% sure that the 
rebase is correct.

Known issues:

* test_traceback and test_extcall fail
* _PyEval_EvalCodeWithName2() should be merged into _PyEval_EvalCodeWithName() 
to factorize the code

I didn't review the change yet.

--
Added file: http://bugs.python.org/file44403/callfunc-6.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27213] Rework CALL_FUNCTION* opcodes

[STINNER Victor]

STINNER Victor added the comment:

I'm working on a new calling convention: "fast call".

I already pushed changes implementing first functions:

_PyObject_FastCallDict(PyObject *func, PyObject **stack, Py_ssize_t nargs, 
PyObject *kwargs)

kwargs is a Python dict, but it can be NULL.

_PyObject_FastCallDict() avoids the creation a temporary tuple when you don't 
have to pass keyword arguments.

But I'm interested by an evolution to avoid also the creation of a temporary 
dictionary: issue #27830, "Add _PyObject_FastCallKeywords(): avoid the creation 
of a temporary dictionary for keyword arguments". This function would be the 
fundation for a new calling convention for C functions: #27810, "Add 
METH_FASTCALL: new calling convention for C functions".

I didn't read your patch carefully, but I would like to make sure that your 
patch would benefit of these APIs to avoid the creation of temporary tuple 
and/or dict.

"func(*args, key=value)" uses CALL_FUNCTION_EX and so requires to create a 
temporary dictionary. Maybe it's ok, I don't think that this syntax is commonly 
used.

"func(*args)" uses CALL_FUNCTION_EX: if args is a tuple, can you confirm that 
it would be possible to use _PyObject_FastCallDict() and/or 
_PyObject_FastCallKeywords() to avoid the creation a new temporary tuple?

Hum, I don't think that "func(*args)" can be optimized if args is a list 
because a list is mutable, we cannot use a direct access to the internal C 
array of a list.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27213] Rework CALL_FUNCTION* opcodes

[STINNER Victor]

STINNER Victor added the comment:

> Pybench is now only ~200ms slower instead of 1200ms slower. But the whole 
> point of this patch is that CALL_FUNCTION_EX shouldn't be optimized for, so 
> I'd much prefer real benchmarking results

FYI I released a first version of the "new" Python benchmark suite: performance 
0.1 (quickly fixed with performance 0.1.1):

* https://pypi.python.org/pypi/performance
* https://github.com/python/benchmarks

The README file contains a few information how to run a stable benchmark. 
Please help me to complete this documentation :-)

I may help you to run these benchmarks.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27213] Rework CALL_FUNCTION* opcodes

[STINNER Victor]

STINNER Victor added the comment:

See also the issue #27809 "_PyObject_FastCall(): add support for keyword 
arguments" where we discuss if we can completely avoid the creation of a 
temporary dictionary to pass keyword arguments.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27213] Rework CALL_FUNCTION* opcodes

[Demur Rumed]

Demur Rumed added the comment:

Since the most common use of CALL_FUNCTION_EX is..

def f(*x,*kw):
other_func(*x, **kw)

I've added some code to BUILD_MAP_UNPACK_WITH_CALL & BUILD_TUPLE_UNPACK to not 
allocate a new object if called with oparg of 1 & TOP() is correct type

--
Added file: http://bugs.python.org/file43741/callfunc5.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27213] Rework CALL_FUNCTION* opcodes

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

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?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27213] Rework CALL_FUNCTION* opcodes

[Demur Rumed]

Demur Rumed added the comment:

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 #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

--
Added file: http://bugs.python.org/file43602/callfunc3.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27213] Rework CALL_FUNCTION* opcodes

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

Added comments on Rietveld.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27213] Rework CALL_FUNCTION* opcodes

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

Issue27358 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)) {

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27213] Rework CALL_FUNCTION* opcodes

[Demur Rumed]

Demur Rumed added the comment:

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

--
Added file: http://bugs.python.org/file43493/callfunc2.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27213] Rework CALL_FUNCTION* opcodes

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

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 issue26814 and issue27128. 
_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).

--
nosy: +haypo

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27213] Rework CALL_FUNCTION* opcodes

[Serhiy Storchaka]

Changes by Serhiy Storchaka :


--
stage:  -> patch review

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27213] Rework CALL_FUNCTION* opcodes

[Mark Shannon]

Mark Shannon added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27213] Rework CALL_FUNCTION* opcodes

[Demur Rumed]

Demur Rumed added the comment:

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

--
keywords: +patch
Added file: http://bugs.python.org/file43474/callfunc.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27213] Rework CALL_FUNCTION* opcodes

[Demur Rumed]

Demur Rumed added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27213] Rework CALL_FUNCTION* opcodes

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

Do you take this for your Demur?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27213] Rework CALL_FUNCTION* opcodes

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27213] Rework CALL_FUNCTION* opcodes

[Demur Rumed]

Demur Rumed added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27213] Rework CALL_FUNCTION* opcodes

[Serhiy Storchaka]

Changes by Serhiy Storchaka :


--
nosy: +Demur Rumed, Mark.Shannon

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27213] Rework CALL_FUNCTION* opcodes

[Serhiy Storchaka]

New submission from Serhiy Storchaka:

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)

1. CALL_FUNCTION takes the fixed number of positional arguments. oparg is the 
number of arguments. The stack contains positional arguments.

2. 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 issue27140).

3. 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:

1. Calling a function with keyword arguments uses less stack and less 
LOAD_CONST instructions.
2. Calling a function with keyword arguments no longer needs EXTENDED_ARG.
3. The number of positional and keyword arguments is no longer limited by 255 
(at least not in the bytecode).
4. 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

--
components: Interpreter Core
messages: 267241
nosy: serhiy.storchaka
priority: normal
severity: normal
status: open
title: Rework CALL_FUNCTION* opcodes
type: enhancement
versions: Python 3.6

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27213] Rework CALL_FUNCTION* opcodes

[Serhiy Storchaka]

Changes by Serhiy Storchaka :


--
dependencies: +Opcode for creating dict with constant keys

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com