Direct calling for variadic C functions

This patch expands direct calling to more functions in the builtin module and adds support for variadic functions.

Changed several functions in list-object and dict-object to use the new calling convention.

slowpickle:
Min: 0.589025 -> 0.513232: 14.77% faster
Avg: 0.602472 -> 0.526819: 14.36% faster
Significant (t=7.409267, a=0.95)
Stddev: 0.07249 -> 0.07191: 0.80% smaller

ai:
Min: 0.448522 -> 0.439669: 2.01% faster
Avg: 0.454284 -> 0.443105: 2.52% faster
Significant (t=8.024329, a=0.95)
Stddev: 0.01048 -> 0.00918: 14.10% smaller


django:
Min: 0.814697 -> 0.820148: 0.66% slower
Avg: 0.817904 -> 0.822301: 0.53% slower
Significant (t=-7.385589, a=0.95)
Stddev: 0.00429 -> 0.00413: 3.75% smaller

[rbp, 2009-11-10 19:18:51]

http://codereview.appspot.com/151055/diff/1011/15
File Include/methodobject.h (right):

http://codereview.appspot.com/151055/diff/1011/15#newcode59
Include/methodobject.h:59: int          ml_arity;      /* Number of parameters
for METH_FIXED and METH_ARGS funcs. */
Perhaps change this to something like "Number of parameters for METH_FIXED and
maximum number of parameters for METH_ARGS" (and indent for 80-col)

http://codereview.appspot.com/151055/diff/1011/12
File Python/bltinmodule.c (right):

http://codereview.appspot.com/151055/diff/1011/12#newcode328
Python/bltinmodule.c:328: locals = Py_None;
Please fix indentation to match the rest of the code (tabs instead of spaces).

http://codereview.appspot.com/151055/diff/1011/12#newcode787
Python/bltinmodule.c:787: locals = NULL;
Please fix indentation to match the rest of the code (tabs instead of spaces).

http://codereview.appspot.com/151055/diff/1011/12#newcode1879
Python/bltinmodule.c:1879: z = Py_None;
Please fix indentation to match the rest of the code (tabs instead of spaces).

http://codereview.appspot.com/151055/diff/1011/12#newcode2363
Python/bltinmodule.c:2363: builtin_raw_input(PyObject *self, PyObject *args)
Couldn't raw_input (and input, accordingly) be included in this patch as well?

http://codereview.appspot.com/151055/diff/1011/12#newcode2621
Python/bltinmodule.c:2621: builtin_sum(PyObject *self, PyObject *args)
Couldn't sum be included in this patch?

http://codereview.appspot.com/151055/diff/1011/12#newcode2907
Python/bltinmodule.c:2907: static PyMethodDef builtin_methods[] = {
Perhaps these could be kept 80-col, by wrapping at the doc string (as in other
files, such as dictobject.c)

[ebo, 2009-11-11 10:14:45]

Included some tests. Fixed the rest of the whitespace problems.

http://codereview.appspot.com/151055/diff/1011/15
File Include/methodobject.h (right):

http://codereview.appspot.com/151055/diff/1011/15#newcode59
Include/methodobject.h:59: int          ml_arity;      /* Number of parameters
for METH_FIXED and METH_ARGS funcs. */
Maybe we can merge METH_FIXED and METH_ARGS that way. If maximal_arity <= arity,
the functions needs a fixed number of parameters.

http://codereview.appspot.com/151055/diff/1011/12
File Python/bltinmodule.c (right):

http://codereview.appspot.com/151055/diff/1011/12#newcode2363
Python/bltinmodule.c:2363: builtin_raw_input(PyObject *self, PyObject *args)
Included.

http://codereview.appspot.com/151055/diff/1011/12#newcode2621
Python/bltinmodule.c:2621: builtin_sum(PyObject *self, PyObject *args)
Included.

http://codereview.appspot.com/151055/diff/1011/12#newcode2907
Python/bltinmodule.c:2907: static PyMethodDef builtin_methods[] = {
Broke all long lines.

[rbp, 2009-11-11 13:39:42]

http://codereview.appspot.com/151055/diff/1011/15
File Include/methodobject.h (right):

http://codereview.appspot.com/151055/diff/1011/15#newcode59
Include/methodobject.h:59: int          ml_arity;      /* Number of parameters
for METH_FIXED and METH_ARGS funcs. */
That's actually how the patch I was working on went about (as well as Neal
Norwitz's, linked to at unladen-swallow's issue 88). We might merge everything
into METH_ARGS (ou METH_ARGRANGE, meaning ARG number is between a defined
RANGE), and rename {min_,}arity to {min,max}_arity.

That would mean always checking "min_arity <= na && na <= max_arity" instead of
"na == arity", but I suppose that's fine. Let me know if you want help writing
this, otherwise I'll just keep reviewing so we don't trample each other.

I'd like to hear from people who worked r890
(http://codereview.appspot.com/144079) about this, since it pokes directly at
their work.

http://codereview.appspot.com/151055/diff/3001/3009
File Objects/listobject.c (right):

http://codereview.appspot.com/151055/diff/3001/3009#newcode920
Objects/listobject.c:920: {
Curly braces should go on the same line as "if" (PEP7). I also saw two more of
these on eval.cc.

-- sorry, forgot to mention that earlier :P

http://codereview.appspot.com/151055/diff/3001/3002
File Python/bltinmodule.c (right):

http://codereview.appspot.com/151055/diff/3001/3002#newcode1565
Python/bltinmodule.c:1565: line = builtin_raw_input(self, args);
This and the function's signature should change to match the modification to
raw_input. It's only nomenclature.

[Collin Winter, 2009-11-11 23:03:08]

http://codereview.appspot.com/151055/diff/1011/15
File Include/methodobject.h (right):

http://codereview.appspot.com/151055/diff/1011/15#newcode59
Include/methodobject.h:59: int          ml_arity;      /* Number of parameters
for METH_FIXED and METH_ARGS funcs. */
On 2009/11/11 13:39:42, rbp wrote:
> That's actually how the patch I was working on went about (as well as Neal
> Norwitz's, linked to at unladen-swallow's issue 88). We might merge everything
> into METH_ARGS (ou METH_ARGRANGE, meaning ARG number is between a defined
> RANGE), and rename {min_,}arity to {min,max}_arity.
> 
> That would mean always checking "min_arity <= na && na <= max_arity" instead
of
> "na == arity", but I suppose that's fine. Let me know if you want help writing
> this, otherwise I'll just keep reviewing so we don't trample each other.
> 
> I'd like to hear from people who worked r890
> (http://codereview.appspot.com/144079) about this, since it pokes directly at
> their work.

That would be me. I'd be happy to see ml_minarity and ml_maxarity ints, the way
Neal Norwitz did it in his original patch. I implemented simple fixed-arity
functions because that was easy, and it established a pattern for this patch.

METH_ARGRANGE should supersede METH_FIXED (ie, METH_FIXED should just go away)
instead of adding a new classification for methods. That might slow down calls
in the interpreter, but it should produce the same LLVM IR.

http://codereview.appspot.com/151055/diff/3020/4012#newcode42
Include/methodobject.h:42: #define PyCFunction_GET_ARITY(func) \
This should be changed to GET_MINARITY (or GET_MIN_ARITY, actually).

http://codereview.appspot.com/151055/diff/3020/4012#newcode59
Include/methodobject.h:59: int          ml_arity;      /* Number of parameters
for METH_FIXED and METH_ARGS funcs. */
Please wrap lines at 80 columns. This applies elsewhere.

http://codereview.appspot.com/151055/diff/3020/4012#newcode74
Include/methodobject.h:74: /* METH_NOARGS, METH_O and METH_FIXED must not be
combined with the flags above.
I'm having a really tough scanning METH_ARGRANGE. METH_ARG_RANGE maybe?

http://codereview.appspot.com/151055/diff/3020/4012#newcode77
Include/methodobject.h:77: #define METH_FIXED    0x0020     /* Function arity =
constant */
I'd say "a known interval" rather than "constant interval".

http://codereview.appspot.com/151055/diff/3020/4012#newcode94
Include/methodobject.h:94: /* Maximum value for ml_arity. */
This should probably be renamed to just PY_MAX_ARITY.

http://codereview.appspot.com/151055/diff/3020/4013
File Objects/methodobject.c (right):

http://codereview.appspot.com/151055/diff/3020/4013#newcode41
Objects/methodobject.c:41: if (ml->ml_arity < 0 || ml->ml_arity >
PY_MAX_FIXED_ARITY ||
Also sanity-check that ml_arity <= ml_maxarity?

http://codereview.appspot.com/151055/diff/3020/4013#newcode95
Objects/methodobject.c:95: int flags = PyCFunction_GET_FLAGS(func) &
~(METH_CLASS | METH_STATIC | METH_COEXIST);
What's the point of this change?

http://codereview.appspot.com/151055/diff/3020/4013#newcode109
Objects/methodobject.c:109: PyObject *args[PY_MAX_FIXED_ARITY] = {NULL, NULL,
NULL};
I don't think its necessary to add more NULLs, but jyasskin can correct me.

http://codereview.appspot.com/151055/diff/3020/4013#newcode126
Objects/methodobject.c:126: if (maxarity < arity)
In what cases will the maximum arity be less than the minimum arity?

http://codereview.appspot.com/151055/diff/3020/4015
File Objects/unicodeobject.c (right):

http://codereview.appspot.com/151055/diff/3020/4015#newcode7996
Objects/unicodeobject.c:7996: {"zfill", (PyCFunction) unicode_zfill,
METH_ARGRANGE, zfill__doc__, /*arity=*/1},
I think the methoddef should have to define both the min and max arity
explicitly.

http://codereview.appspot.com/151055/diff/3020/4009
File Python/bltinmodule.c (right):

http://codereview.appspot.com/151055/diff/3020/4009#newcode164
Python/bltinmodule.c:164: builtin_apply(PyObject *self, PyObject *func, PyObject
*alist, PyObject *kwdict)
Is apply() called enough to warrant this modification? This applies to most of
the functions modified below. Which benchmarks does this help?

http://codereview.appspot.com/151055/diff/3020/4009#newcode559
Python/bltinmodule.c:559: long x = PyInt_AsLong(arg);
Curious: does this change the error message you get from chr(object())?

http://codereview.appspot.com/151055/diff/3020/4010
File Python/llvm_fbuilder.cc (right):

http://codereview.appspot.com/151055/diff/3020/4010#newcode1701
Python/llvm_fbuilder.cc:1701: // Only optimize calls to C functions with a fixed
number of parameters,
This comment needs to be updated.

http://codereview.appspot.com/151055/diff/3020/4018
File Util/ConstantMirror.h (right):

http://codereview.appspot.com/151055/diff/3020/4018#newcode70
Util/ConstantMirror.h:70: // pointer. This currently only works for
METH_ARGRANGE functions.
Comment what 'arity' means, now that this is intended for variadic functions.
Rename it to max_arity?

http://codereview.appspot.com/151055/diff/3020/4019
File Util/RuntimeFeedback.cc (right):

http://codereview.appspot.com/151055/diff/3020/4019#newcode27
Util/RuntimeFeedback.cc:27: if (this->flags & METH_ARGRANGE)
Please unify this under a single "this->flags & METH_ARGRANGE" test.

http://codereview.appspot.com/151055/diff/3020/4019#newcode29
Util/RuntimeFeedback.cc:29: if (this->maxarity < this->arity)
This test should not be repeated all over the place. This kind of normalization
should be done once, at the earliest possible opportunity.

http://codereview.appspot.com/151055/diff/3020/4020
File Util/RuntimeFeedback.h (right):

http://codereview.appspot.com/151055/diff/3020/4020#newcode53
Util/RuntimeFeedback.h:53: int arity;
Convert these ints to shorts to save memory? Probably should do the same thing
for the method definition structs.

[ebo, 2009-11-12 13:06:44]

PTAL

http://codereview.appspot.com/151055/diff/3020/4013
File Objects/methodobject.c (right):

http://codereview.appspot.com/151055/diff/3020/4013#newcode41
Objects/methodobject.c:41: if (ml->ml_arity < 0 || ml->ml_arity >
PY_MAX_FIXED_ARITY ||
done.

http://codereview.appspot.com/151055/diff/3020/4013#newcode95
Objects/methodobject.c:95: int flags = PyCFunction_GET_FLAGS(func) &
~(METH_CLASS | METH_STATIC | METH_COEXIST);
Little leftover from before.

http://codereview.appspot.com/151055/diff/3020/4013#newcode126
Objects/methodobject.c:126: if (maxarity < arity)
When you only fill (min)arity. Backward compatibility. Removed it. You have to
fill both values now.

http://codereview.appspot.com/151055/diff/3020/4015
File Objects/unicodeobject.c (right):

http://codereview.appspot.com/151055/diff/3020/4015#newcode7996
Objects/unicodeobject.c:7996: {"zfill", (PyCFunction) unicode_zfill,
METH_ARGRANGE, zfill__doc__, /*arity=*/1},
Done.

http://codereview.appspot.com/151055/diff/3020/4009
File Python/bltinmodule.c (right):

http://codereview.appspot.com/151055/diff/3020/4009#newcode164
Python/bltinmodule.c:164: builtin_apply(PyObject *self, PyObject *func, PyObject
*alist, PyObject *kwdict)
I don't know. I just changed all functions in builtin. I guessed it couldn't
hurt.

http://codereview.appspot.com/151055/diff/3020/4009#newcode559
Python/bltinmodule.c:559: long x = PyInt_AsLong(arg);
No, it doesn't.

http://codereview.appspot.com/151055/diff/3020/4019
File Util/RuntimeFeedback.cc (right):

http://codereview.appspot.com/151055/diff/3020/4019#newcode27
Util/RuntimeFeedback.cc:27: if (this->flags & METH_ARGRANGE)
On 2009/11/11 23:03:08, Collin Winter wrote:
> Please unify this under a single "this->flags & METH_ARGRANGE" test.

Done.

http://codereview.appspot.com/151055/diff/3020/4019#newcode29
Util/RuntimeFeedback.cc:29: if (this->maxarity < this->arity)
On 2009/11/11 23:03:08, Collin Winter wrote:
> This test should not be repeated all over the place. This kind of
normalization
> should be done once, at the earliest possible opportunity.

Done.

http://codereview.appspot.com/151055/diff/3020/4020
File Util/RuntimeFeedback.h (right):

http://codereview.appspot.com/151055/diff/3020/4020#newcode53
Util/RuntimeFeedback.h:53: int arity;
Done.

[Jeffrey Yasskin, 2009-11-12 17:47:12]

http://codereview.appspot.com/151055/diff/3020/4013
File Objects/methodobject.c (right):

http://codereview.appspot.com/151055/diff/3020/4013#newcode109
Objects/methodobject.c:109: PyObject *args[PY_MAX_FIXED_ARITY] = {NULL, NULL,
NULL};
On 2009/11/11 23:03:08, Collin Winter wrote:
> I don't think its necessary to add more NULLs, but jyasskin can correct me.

That's correct. The compiler fills in the rest of the array with 0s if you don't
fill it explicitly. We could actually initialize it with {} instead of {NULL},
but I like the little reminder that that's what it's getting filled with.

[rbp, 2009-11-12 20:51:06]

I'm still compiling to test properly, but I have to get away for about an hour
and Collin mentioned that he was going to commit this soon, so I just wanted to
get these comments in (he's probably already spotted them all anyway).

Mostly whitespace and nomenclature, LGTM otherwise.

http://codereview.appspot.com/151055/diff/3023/3027
File Include/methodobject.h (right):

http://codereview.appspot.com/151055/diff/3023/3027#newcode41
Include/methodobject.h:41: /* GET_ARITY only makes sense for METH_ARG_RANGE
functions. */
GET_ARITY is no more, right? This should be "GET_{MIN,MAX}_ARITY"

http://codereview.appspot.com/151055/diff/3023/3027#newcode44
Include/methodobject.h:44: (((PyCFunctionObject *)func) -> m_ml -> ml_minarity))
Should we use "min_arity" instead of "minarity" (and max_arity accordingly), to
match GET_MIN_ARITY? Seems more consistent (and  I keep reading "minority" :P).

http://codereview.appspot.com/151055/diff/3023/3027#newcode45
Include/methodobject.h:45: /* GET_MAXARITY only makes sense for METH_ARG_RANGE
functions. */
That's "GET_MAX_ARITY" (there's an underscore missing)

http://codereview.appspot.com/151055/diff/3023/3027#newcode62
Include/methodobject.h:62: for METH_ARG_RANGE funcs. */
Please fix whitespace (between "short" and "ml_", and on continuation comment
lines)

http://codereview.appspot.com/151055/diff/3023/3028
File Objects/methodobject.c (right):

http://codereview.appspot.com/151055/diff/3023/3028#newcode34
Objects/methodobject.c:34: will fill in ml_minarity = ml_maxarity = 0 for us. */
Please wrap at 80 columns

http://codereview.appspot.com/151055/diff/3023/3028#newcode47
Objects/methodobject.c:47: return NULL;
Please indent these continuation lines (and the three subsequent lines)
properly.

http://codereview.appspot.com/151055/diff/3023/3028#newcode108
Objects/methodobject.c:108: int arity = PyCFunction_GET_MIN_ARITY(func);
"arity" should be minarity now, right?

http://codereview.appspot.com/151055/diff/3023/3028#newcode110
Objects/methodobject.c:110: PyObject *args[PY_MAX_ARITY] = {NULL, NULL, NULL};
Hadn't we agreed on keeping only {NULL}? Listing three hardcodes PY_MAX_ARITY
(in yet another place).

http://codereview.appspot.com/151055/diff/3023/3024
File Python/bltinmodule.c (right):

http://codereview.appspot.com/151055/diff/3023/3024#newcode2991
Python/bltinmodule.c:2991: {"#@exec",	        (PyCFunction)builtin_exec,	   
METH_ARG_RANGE,
Please wrap at 80 columns

http://codereview.appspot.com/151055/diff/3023/3024#newcode2997
Python/bltinmodule.c:2997: {"#@import_star",	(PyCFunction)builtin_import_star,	 
  METH_O,
Please wrap at 80 columns

http://codereview.appspot.com/151055/diff/3023/3026
File Python/eval.cc (right):

http://codereview.appspot.com/151055/diff/3023/3026#newcode4068
Python/eval.cc:4068: else if (minarity == 0)
Please fix indentation (to match "PyExc_TypeError", as on the lines below)

http://codereview.appspot.com/151055/diff/3023/3026#newcode4218
Python/eval.cc:4218: PyObject *args[PY_MAX_ARITY] = {NULL, NULL, NULL};
Same as I mentione somewhere else: shouldn't we just leave this at {NULL}?

http://codereview.appspot.com/151055/diff/3023/3025
File Python/llvm_fbuilder.cc (right):

http://codereview.appspot.com/151055/diff/3023/3025#newcode1788
Python/llvm_fbuilder.cc:1788: // PyObject *. If the function was tagged with
METH_ARG_RANGE and arity=0, we
Hmmm... What's the policy on this (81 columns)? I'd be tempted to wrap at 80
columns (and change the following 4 comment lines accordingly).

[Collin Winter, 2009-11-14 00:29:03]

I cleaned up the style issues and fixed a failing assertion; committed in r894.

Thanks for the patch!