Author vstinner
Recipients serhiy.storchaka, vstinner, yselivanov, ztane
Date 2016-08-16.21:05:56
SpamBayes Score -1.0
Marked as misclassified Yes
Message-id <CAMpsgwbVTogqa7bNcuF-9vckGnDqevwUFLodmzT1MQQ5ATmG8Q@mail.gmail.com>
In-reply-to <1470945903.3.0.476843554273.issue27128@psf.upfronthosting.co.za>
Content
Serhiy Storchaka added the comment:
>> Do you suggest to not add these 2 new functions?
>
> Yes, I suggest to not add them. The API for calling is already too large.
> Internally we can directly use _PyObject_FastCall(), and third party code
> should get benefit from optimized PyObject_CallFunctionObjArgs().

Well, we can start without them, and see later if it's worth it.

I didn't propose to add new functions to make the code faster, but to
make the API simpler.

I dislike PyEval_CallObjectWithKeywords(func, arg, kw) because it has
a special case if arg is a tuple. If arg is a tuple, the tuple is
unpacked. It already leaded to a complex and very bug in the
implementation of generators! See the issue #21209. I'm not sure that
such use case is well known and understood by everyone...

It's common to call a function with no argument or just one argument,
so I proposed to add an obvious and simple API for these common cases.
Well, again, I will open a new issue to discuss that.

>> > Can existing function PyObject_Call() be optimized to achieve a
>> > comparable benefit?
>> Sorry, I don't understand. This function requires a tuple. The whole
>> purpose of my patch is to avoid temporary tuples.
>
> Sorry, I meant PyObject_CallFunctionObjArgs() and like.

Yes, my full patch does optimize these functions:
https://hg.python.org/sandbox/fastcall/file/2dc558e01e66/Objects/abstract.c#l2523

>> Keyword arguments are optional. Having support for them cost nothing when
>> they are not used.
>
> My point is that if keyword arguments are used, this is not a fast call, and
> should use old calling protocol. The overhead of creating a tuple for args is
> dwarfen by the overhead of creating a dict for kwargs and parsing it.

I'm not sure that I understand your point.

For example, in my full patch, I have a METH_FASTCALL calling
convention for C functions. With this calling convention, a function
accepts positional arguments and keyword arguments. If you don't pass
keyword arguments, the call should be faster according to my
benchmarks.

How do you want to implement METH_FASTCALL if you cannot pass keyword
arguments? Does it mean that METH_FASTCALL can only be used by the
functions which don't accept keyword arguments at all?

It's ok if passing keyword arguments is not faster, but simply as fast
as before, if the "positional arguments only" case is faster, no?

>> I really want to have a "pystack" API. In this patch, the new file looks
>> useless, but in the full patch there are many functions including a few
>> complex functions. I prefer to add the file now and complete it later.
>
> But for now there is no a "pystack" API. What do you want to add?

See my fastcall branch:

https://hg.python.org/sandbox/fastcall/file/2dc558e01e66/Include/pystack.h
https://hg.python.org/sandbox/fastcall/file/2dc558e01e66/Python/pystack.c

All these functions are private. They are used internally to implement
all functions of the Python C API to call functions.

> On other side, other code can get a benefit from using _PyTuple_FromArray().

Ah? Maybe you should open a different issue for that.

I prefer to have an API specific to build a "stack" to call functions.

> Here is alternative simplified patch.
>
> 1) _PyStack_AsTuple() is renamed to _PyTuple_FromArray() (-2 new files).
> 2) Optimized PyObject_CallFunctionObjArgs(), PyObject_CallMethodObjArgs() and
> _PyObject_CallMethodIdObjArgs().

My full patch does optimize "everything", it's deliberate to start
with something useless but short.

> 5) Reverted changes in Objects/descrobject.c. They added a regression in
> namedtuple attributes access.

Ah? What is the regression?
History
Date User Action Args
2016-08-16 21:05:56vstinnersetrecipients: + vstinner, serhiy.storchaka, yselivanov, ztane
2016-08-16 21:05:56vstinnerlinkissue27128 messages
2016-08-16 21:05:56vstinnercreate