Author vstinner
Recipients serhiy.storchaka, vstinner, yselivanov
Date 2016-08-09.21:30:24
SpamBayes Score -1.0
Marked as misclassified Yes
Message-id <CAMpsgwYuO8JSCCrMQ5e0Du_Bmm1Bg4B-mOc3GaDxBKeOXvhxwQ@mail.gmail.com>
In-reply-to <1470771950.51.0.249852514927.issue27128@psf.upfronthosting.co.za>
Content
> Benchmarking results look nice, but despite the fact that this patch is
only small part of issue26814, it looks to me larger that it could be.

Oh I failed to express my intent. This initial patch is not expected to
introduce any speedup. In fact I noticed major performance regressions on
the CPython benchmark suite using my full fastcall patch. It took me time
to understand that they are more issues with benchmarks than my work. This
minimum patch only adds new functions but don't really use them. I patched
a few functions to show how the new functions can be used. I spent most of
my time just to ensure that the minimum patch doesn't introduce performance
regression.

> 1. The patch includes two parts: adding _PyObject_FastCall() and adding
PyObject_CallNoArg() and PyObject_CallArg1(). How large the role of latter
functions in the speed up?

See my remark above, no speedup is expected.

Do you suggest to not add these 2 new functions? Since they are well
defined and simple, I chose to make them public. Their API is nicer than
_PyObject_Call().

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

In my full patch, PyObject_Call() calls _PyObject_FastCall() in most cases.

> 2. I think that supporting keyword arguments in _PyObject_FastCall()
doesn't make much sense now.

Well, I can add support for keyword arguments later and start with an
assertion (fail if they are used). But I really need them in the API, and I
don't want to change to API later.

I plan to add a new METH_FASTCALL calling convention for C functions. I
would prefer to not have two new calling conventions, but use Argument
Clinic to emit efficient code to parse arguments.

> Calling with keyword arguments adds such much overhead, that it dwarfs
the benefit of avoiding the creation of one tuple. I think that the patch
can be simpler if drop the support of keyword arguments.

Keyword arguments are optional. Having support for them cost nothing when
they are not used.

> 3. The patch adds two files for one function _PyStack_AsTuple(). I would
prefer something like _PyTuple_FromArray(). It could be used in other
places, not just in argument parsing.

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.

I'm limited by Mercurial and our workflow (tools), it would be much easier
to explain my work using a patch serie, but it's not possible to publish a
patch serie...
History
Date User Action Args
2016-08-09 21:30:25vstinnersetrecipients: + vstinner, serhiy.storchaka, yselivanov
2016-08-09 21:30:25vstinnerlinkissue27128 messages
2016-08-09 21:30:24vstinnercreate