Add support for fixed-arity functions with arity greater than METH_O

Committed in r890.

Benchmarking vs http://codereview.appspot.com/144078. This was implemented to help Django, and accordingly it benefits the most.

django:
Min: 0.925832 -> 0.866477: 6.85% faster
Avg: 0.929429 -> 0.870545: 6.76% faster
Significant (t=75.111745, a=0.95)
Stddev: 0.00525 -> 0.00582: 9.72% larger

2to3:
Min: 32.098121 -> 31.701180: 1.25% faster
Avg: 32.125116 -> 31.745574: 1.20% faster
Significant (t=10.993610, a=0.95)
Stddev: 0.02732 -> 0.07220: 62.17% larger

ai:
Min: 0.460347 -> 0.451982: 1.85% faster
Avg: 0.463752 -> 0.455268: 1.86% faster
Significant (t=5.747795, a=0.95)
Stddev: 0.01022 -> 0.01065: 4.11% larger

rietveld:
Min: 0.504553 -> 0.490781: 2.81% faster
Avg: 0.613619 -> 0.601696: 1.98% faster
Not significant
Stddev: 0.18400 -> 0.18798: 2.11% larger

slowpickle:
Min: 0.599689 -> 0.588905: 1.83% faster
Avg: 0.616109 -> 0.604985: 1.84% faster
Not significant
Stddev: 0.08503 -> 0.08518: 0.18% larger

slowspitfire:
Min: 0.619873 -> 0.624143: 0.68% slower
Avg: 0.621270 -> 0.626009: 0.76% slower
Significant (t=-8.066078, a=0.95)
Stddev: 0.00167 -> 0.00563: 70.44% larger

slowunpickle:
Min: 0.263925 -> 0.264087: 0.06% slower
Avg: 0.276726 -> 0.277567: 0.30% slower
Not significant
Stddev: 0.04327 -> 0.04422: 2.13% larger

[Collin Winter, 2009-11-03 22:28:51]

PTAL

[Jeffrey Yasskin, 2009-11-04 01:06:39]

I'm going to be gone tomorrow, but if Neal becomes happy with this, go ahead and
check it in.

http://codereview.appspot.com/144079/diff/24/25
File Include/methodobject.h (right):

http://codereview.appspot.com/144079/diff/24/25#newcode41
Line 41: /* GET_ARITY only makes sense for METH_FIXED functions. */
It'd be nice to assert this. Since assert() is an expression, you probably can.

http://codereview.appspot.com/144079/diff/24/25#newcode54
Line 54: int          ml_arity;      /* Number of parameters (see METH_FIXED) */
The "see METH_FIXED" appears to be a dangling link.

http://codereview.appspot.com/144079/diff/24/25#newcode71
Line 71: #define METH_FIXED    0x0010  /* Function arity = constant */
Arguably, you could just replace METH_NOARGS with METH_FIXED. ml_arity will be 0
by default if it's not specified in an argument list.

http://codereview.appspot.com/144079/diff/24/25#newcode76
Line 76: #define METH_CLASS    0x0020
If we're going to break ABI compatibility here, should we add some blank bits to
allow for future expansion?

http://codereview.appspot.com/144079/diff/24/26
File Lib/test/test_llvm.py (right):

http://codereview.appspot.com/144079/diff/24/26#newcode2592
Line 2592: foo = compile_for_llvm('foo', 'def foo(): import os',
setattr might be more future-proof here, since iirc you're turning import back
into an opcode.

http://codereview.appspot.com/144079/diff/24/27
File Objects/methodobject.c (right):

http://codereview.appspot.com/144079/diff/24/27#newcode119
Line 119: else if (size == 3 && arity == 3)
I believe it's legal to pass more arguments to a C function than it expects
(otherwise, the 0-arg technique of passing an extra NULL would break). So you
can handle the <3-arg case by using something like:

PyObject *args[3] = {0};
switch(size) {
  default:  /* die */
  case 3:
    args[2] = PyTuple_GET_ITEM(arg, 2);
    /* fallthrough */
  case 2:
    args[1] = PyTuple_GET_ITEM(arg, 1);
    /* fallthrough */
  case 1:
    args[0] = PyTuple_GET_ITEM(arg, 0);
    /* fallthrough */
  case 0:
    break;
}
if (size == arity)
  return ((PyCFunctionThreeArgs)meth)(self, args[0], args[1], args[2]);
else
  PyErr_Format...

http://codereview.appspot.com/144079/diff/24/28
File Python/bltinmodule.c (right):

http://codereview.appspot.com/144079/diff/24/28#newcode2989
Line 2989: {"hasattr",	(PyCFunction)builtin_hasattr,    METH_FIXED, hasattr_doc,
2},
Comment the meaning of the "2", and maybe wrap for 80 columns.

http://codereview.appspot.com/144079/diff/24/29
File Python/eval.cc (right):

http://codereview.appspot.com/144079/diff/24/29#newcode4233
Line 4233: C_TRACE(x, (*(PyCFunctionThreeArgs)meth)(self,
Similarly here, it's probably better just to have one call with extra arguments.

http://codereview.appspot.com/144079/diff/24/30
File Python/llvm_fbuilder.cc (right):

http://codereview.appspot.com/144079/diff/24/30#newcode1799
Line 1799: std::vector<Value*> args;
Use an llvm::SmallVector<Value*, MAX_ARITY>.

http://codereview.appspot.com/144079/diff/24/30#newcode1819
Line 1819: for (int i = 1; i <= num_args; ++i) {  // Don't decref args[0]
Why not?

http://codereview.appspot.com/144079/diff/24/30#newcode3546
Line 3546: LlvmFunctionBuilder::GetNull()
Yeah, that's a very nice function.

http://codereview.appspot.com/144079/diff/24/32
File Util/ConstantMirror.cc (right):

http://codereview.appspot.com/144079/diff/24/32#newcode408
Line 408: #define ONE_ARG  PyObject*(PyObject*, PyObject*)
I think you can use the PyCFunctionNArgs typedefs you added in methodobject.h.

http://codereview.appspot.com/144079/diff/24/33
File Util/ConstantMirror.h (right):

http://codereview.appspot.com/144079/diff/24/33#newcode70
Line 70: // pointer. This currently only works for METH_FIXED functions.
It should assert that.

[Reid Kleckner, 2009-11-04 02:49:57]

http://codereview.appspot.com/144079/diff/24/30
File Python/llvm_fbuilder.cc (right):

http://codereview.appspot.com/144079/diff/24/30#newcode3546
Line 3546: LlvmFunctionBuilder::GetNull()
On 2009/11/04 01:06:39, Jeffrey Yasskin wrote:
> Yeah, that's a very nice function.

Oh great C++ expert: would it be possible to turn TypeBuilder into an object
that you construct with a context and then call templatized methods on? 
Something like this: this->_type_builder.get<PyObject*>() ?  Or can you not
create specialized method templates outside of the class declaration?

[Collin Winter, 2009-11-04 21:15:08]

http://codereview.appspot.com/144079/diff/24/25
File Include/methodobject.h (right):

http://codereview.appspot.com/144079/diff/24/25#newcode41
Line 41: /* GET_ARITY only makes sense for METH_FIXED functions. */
On 2009/11/04 01:06:39, Jeffrey Yasskin wrote:
> It'd be nice to assert this. Since assert() is an expression, you probably
can.

assert() may be an expression, but it's a void expression, which causes fun
"void value not ignored" errors. If you have some magic way of getting around
those, I'll implement this.

http://codereview.appspot.com/144079/diff/24/25#newcode54
Line 54: int          ml_arity;      /* Number of parameters (see METH_FIXED) */
On 2009/11/04 01:06:39, Jeffrey Yasskin wrote:
> The "see METH_FIXED" appears to be a dangling link.

Done.

http://codereview.appspot.com/144079/diff/24/25#newcode71
Line 71: #define METH_FIXED    0x0010  /* Function arity = constant */
On 2009/11/04 01:06:39, Jeffrey Yasskin wrote:
> Arguably, you could just replace METH_NOARGS with METH_FIXED. ml_arity will be
0
> by default if it's not specified in an argument list.

Is that default behaviour in the standard, or an accident of gcc? I was trying
to be conservative to avoid breaking MSVC, etc.

http://codereview.appspot.com/144079/diff/24/25#newcode76
Line 76: #define METH_CLASS    0x0020
On 2009/11/04 01:06:39, Jeffrey Yasskin wrote:
> If we're going to break ABI compatibility here, should we add some blank bits
to
> allow for future expansion?

Done.

http://codereview.appspot.com/144079/diff/24/26
File Lib/test/test_llvm.py (right):

http://codereview.appspot.com/144079/diff/24/26#newcode2592
Line 2592: foo = compile_for_llvm('foo', 'def foo(): import os',
On 2009/11/04 01:06:39, Jeffrey Yasskin wrote:
> setattr might be more future-proof here, since iirc you're turning import back
> into an opcode.

Done.

http://codereview.appspot.com/144079/diff/24/27
File Objects/methodobject.c (right):

http://codereview.appspot.com/144079/diff/24/27#newcode119
Line 119: else if (size == 3 && arity == 3)
On 2009/11/04 01:06:39, Jeffrey Yasskin wrote:
> I believe it's legal to pass more arguments to a C function than it expects
> (otherwise, the 0-arg technique of passing an extra NULL would break). So you
> can handle the <3-arg case by using something like:
> 
> PyObject *args[3] = {0};
> switch(size) {
>   default:  /* die */
>   case 3:
>     args[2] = PyTuple_GET_ITEM(arg, 2);
>     /* fallthrough */
>   case 2:
>     args[1] = PyTuple_GET_ITEM(arg, 1);
>     /* fallthrough */
>   case 1:
>     args[0] = PyTuple_GET_ITEM(arg, 0);
>     /* fallthrough */
>   case 0:
>     break;
> }
> if (size == arity)
>   return ((PyCFunctionThreeArgs)meth)(self, args[0], args[1], args[2]);
> else
>   PyErr_Format...

Done. I like the resulting code better (Duff++), but it makes me a sad panda
that C supports that.

http://codereview.appspot.com/144079/diff/24/28
File Python/bltinmodule.c (right):

http://codereview.appspot.com/144079/diff/24/28#newcode2989
Line 2989: {"hasattr",	(PyCFunction)builtin_hasattr,    METH_FIXED, hasattr_doc,
2},
On 2009/11/04 01:06:39, Jeffrey Yasskin wrote:
> Comment the meaning of the "2", and maybe wrap for 80 columns.

Commented. There's enough 80-col violations in this section that I'm not going
to reflow it in this CL.

http://codereview.appspot.com/144079/diff/24/29
File Python/eval.cc (right):

http://codereview.appspot.com/144079/diff/24/29#newcode4233
Line 4233: C_TRACE(x, (*(PyCFunctionThreeArgs)meth)(self,
On 2009/11/04 01:06:39, Jeffrey Yasskin wrote:
> Similarly here, it's probably better just to have one call with extra
arguments.

Done.

http://codereview.appspot.com/144079/diff/24/30
File Python/llvm_fbuilder.cc (right):

http://codereview.appspot.com/144079/diff/24/30#newcode1799
Line 1799: std::vector<Value*> args;
On 2009/11/04 01:06:39, Jeffrey Yasskin wrote:
> Use an llvm::SmallVector<Value*, MAX_ARITY>.

Done.

http://codereview.appspot.com/144079/diff/24/30#newcode1819
Line 1819: for (int i = 1; i <= num_args; ++i) {  // Don't decref args[0]
On 2009/11/04 01:06:39, Jeffrey Yasskin wrote:
> Why not?

Done.

http://codereview.appspot.com/144079/diff/24/32
File Util/ConstantMirror.cc (right):

http://codereview.appspot.com/144079/diff/24/32#newcode408
Line 408: #define ONE_ARG  PyObject*(PyObject*, PyObject*)
On 2009/11/04 01:06:39, Jeffrey Yasskin wrote:
> I think you can use the PyCFunctionNArgs typedefs you added in methodobject.h.

That causes PyTypeBuilder to return PointerTypes instead of the FunctionTypes I
need.

http://codereview.appspot.com/144079/diff/24/33
File Util/ConstantMirror.h (right):

http://codereview.appspot.com/144079/diff/24/33#newcode70
Line 70: // pointer. This currently only works for METH_FIXED functions.
On 2009/11/04 01:06:39, Jeffrey Yasskin wrote:
> It should assert that.

I can do that, but it would mean passing in the method object's flags solely to
implement that assert.

[James Abbatiello, 2009-11-05 03:41:25]

http://codereview.appspot.com/144079/diff/24/25
File Include/methodobject.h (right):

http://codereview.appspot.com/144079/diff/24/25#newcode41
Line 41: /* GET_ARITY only makes sense for METH_FIXED functions. */
On 2009/11/04 21:15:08, Collin Winter wrote:
> If you have some magic way of getting around
> those, I'll implement this.
How about
(assert(((PyCFunctionObject *)func) -> ml -> ml_flags & METH_FIXED),
((PyCFunctionObject *)func) -> m_ml -> ml_arity)

[Jeffrey Yasskin, 2009-11-05 18:08:35]

LGTM

http://codereview.appspot.com/144079/diff/24/25
File Include/methodobject.h (right):

http://codereview.appspot.com/144079/diff/24/25#newcode41
Line 41: /* GET_ARITY only makes sense for METH_FIXED functions. */
On 2009/11/05 03:41:25, James Abbatiello wrote:
> On 2009/11/04 21:15:08, Collin Winter wrote:
> > If you have some magic way of getting around
> > those, I'll implement this.
> How about
> (assert(((PyCFunctionObject *)func) -> ml -> ml_flags & METH_FIXED),
> ((PyCFunctionObject *)func) -> m_ml -> ml_arity)
> 

Yep, the comma operator should work.

http://codereview.appspot.com/144079/diff/24/25#newcode71
Line 71: #define METH_FIXED    0x0010  /* Function arity = constant */
On 2009/11/04 21:15:08, Collin Winter wrote:
> On 2009/11/04 01:06:39, Jeffrey Yasskin wrote:
> > Arguably, you could just replace METH_NOARGS with METH_FIXED. ml_arity will
be
> 0
> > by default if it's not specified in an argument list.
> 
> Is that default behaviour in the standard, or an accident of gcc? I was trying
> to be conservative to avoid breaking MSVC, etc.

Setting all unspecified struct fields to 0 is standard behavior. "If there are
fewer initializers in a brace-enclosed list than there are elements or members
of an aggregate, or fewer characters in a string literal used to initialize an
array of known size than there are elements in the array, the remainder of the
aggregate shall be initialized implicitly the same as objects that have static
storage duration."

http://codereview.appspot.com/144079/diff/24/32
File Util/ConstantMirror.cc (right):

http://codereview.appspot.com/144079/diff/24/32#newcode408
Line 408: #define ONE_ARG  PyObject*(PyObject*, PyObject*)
On 2009/11/04 21:15:08, Collin Winter wrote:
> On 2009/11/04 01:06:39, Jeffrey Yasskin wrote:
> > I think you can use the PyCFunctionNArgs typedefs you added in
methodobject.h.
> 
> That causes PyTypeBuilder to return PointerTypes instead of the FunctionTypes
I
> need.

typename remove_pointer<PyCFunctionThreeArgs>::type ... never mind then.

http://codereview.appspot.com/144079/diff/24/33
File Util/ConstantMirror.h (right):

http://codereview.appspot.com/144079/diff/24/33#newcode70
Line 70: // pointer. This currently only works for METH_FIXED functions.
On 2009/11/04 21:15:08, Collin Winter wrote:
> On 2009/11/04 01:06:39, Jeffrey Yasskin wrote:
> > It should assert that.
> 
> I can do that, but it would mean passing in the method object's flags solely
to
> implement that assert.

Oops, I forgot that the CFunction doesn't include its flags.

http://codereview.appspot.com/144079/diff/2001/2004
File Objects/methodobject.c (right):

http://codereview.appspot.com/144079/diff/2001/2004#newcode113
Line 113: default: abort();
Py_BadInternalCall()?

[nnorwitz, 2009-11-05 18:59:22]

http://codereview.appspot.com/144079/diff/2001/2002
File Include/methodobject.h (right):

http://codereview.appspot.com/144079/diff/2001/2002#newcode54
Line 54: int          ml_arity;      /* Number of parameters for METH_FIXED
funcs. */
You could get by without adding this field by encoding the arity in the METH_
flag below.  Right now, it would take up 2 extra bits.  To support 8 parameters
would take 3 bits.  There is easily enough room.  I'm not sure it's worth doing
though.  I wonder how many methods are in a typical system.  I don't imagine
there are more than 10,000 typically which means this would add at most 40k.

http://codereview.appspot.com/144079/diff/2001/2002#newcode72
Line 72: #define METH_FIXED    0x0040  /* Function arity = constant */
One thing my original patch did is to support variable arguments too (IIRC).  I
don't think it's worth doing in this first pass, but there are quite a number of
APIs that could use this.  e.g., {}.get()

http://codereview.appspot.com/144079/diff/2001/2004
File Objects/methodobject.c (right):

http://codereview.appspot.com/144079/diff/2001/2004#newcode42
Line 42: ml->ml_arity = 1;
These two conditions could be handled as one, but I'm not sure it's worth it.  I
don't know if people would find the condensed code more readable:

         if (ml->ml_flags & (METH_NOARGS|METH_O)) {
                 ml->ml_flags &= ~(METH_NOARGS|METH_O);
                 ml->ml_flags |= METH_FIXED;
                 ml->ml_arity = ((ml->ml_flags & (METH_NOARGS|METH_O)) >> 4) -
1;
         }

http://codereview.appspot.com/144079/diff/2001/2004#newcode125
Line 125: Yes, but C allows this. Go C. */
Note: there are some systems where this crashes.  There was a patch a while back
that changed lots of 

PyObject* Foo(PyObject* self) {}

to

PyObject* Foo(PyObject* self, PyObject* noargs) {}

because some compiler/system crashed without it.  I'm pretty sure this is still
in the tracker.  I don't think you should worry about it, just pointing out that
even though it's supposed to work, sometimes it doesn't.

http://codereview.appspot.com/144079/diff/2001/2004#newcode147
Line 147: to convert them to METH_FIXED and set ml_arity correctly. */
Note this will be reached by any module that hasn't been recompiled and uses
these flags (which is likely a large number).

You should make sure to change the module API version number  (in
Include/modsupport.h) if you didn't.

http://codereview.appspot.com/144079/diff/2001/2006
File Python/eval.cc (right):

http://codereview.appspot.com/144079/diff/2001/2006#newcode4214
Line 4214: default: abort();
raise SystemError

[Collin Winter, 2009-11-05 19:15:45]

Committing.

http://codereview.appspot.com/144079/diff/24/25
File Include/methodobject.h (right):

http://codereview.appspot.com/144079/diff/24/25#newcode41
Line 41: /* GET_ARITY only makes sense for METH_FIXED functions. */
On 2009/11/05 18:08:35, Jeffrey Yasskin wrote:
> On 2009/11/05 03:41:25, James Abbatiello wrote:
> > On 2009/11/04 21:15:08, Collin Winter wrote:
> > > If you have some magic way of getting around
> > > those, I'll implement this.
> > How about
> > (assert(((PyCFunctionObject *)func) -> ml -> ml_flags & METH_FIXED),
> > ((PyCFunctionObject *)func) -> m_ml -> ml_arity)
> > 
> 
> Yep, the comma operator should work.

I had tried that, but I figured out what I was doing wrong. I had


#define PyCFunction_GET_ARITY(func) \
	assert(PyCFunction_GET_FLAGS(func) & METH_FIXED), \
	(((PyCFunctionObject *)func) -> m_ml -> ml_arity)

instead of the correct

#define PyCFunction_GET_ARITY(func) \
	(assert(PyCFunction_GET_FLAGS(func) & METH_FIXED), \
	(((PyCFunctionObject *)func) -> m_ml -> ml_arity))

Done.

http://codereview.appspot.com/144079/diff/24/25#newcode71
Line 71: #define METH_FIXED    0x0010  /* Function arity = constant */
On 2009/11/05 18:08:35, Jeffrey Yasskin wrote:
> On 2009/11/04 21:15:08, Collin Winter wrote:
> > On 2009/11/04 01:06:39, Jeffrey Yasskin wrote:
> > > Arguably, you could just replace METH_NOARGS with METH_FIXED. ml_arity
will
> be
> > 0
> > > by default if it's not specified in an argument list.
> > 
> > Is that default behaviour in the standard, or an accident of gcc? I was
trying
> > to be conservative to avoid breaking MSVC, etc.
> 
> Setting all unspecified struct fields to 0 is standard behavior. "If there are
> fewer initializers in a brace-enclosed list than there are elements or members
> of an aggregate, or fewer characters in a string literal used to initialize an
> array of known size than there are elements in the array, the remainder of the
> aggregate shall be initialized implicitly the same as objects that have static
> storage duration."

Ok. I've removed a constant associated with METH_NOARGS and #defined it to be
METH_FIXED.

http://codereview.appspot.com/144079/diff/2001/2002#newcode72
Line 72: 
On 2009/11/05 18:59:22, nnorwitz wrote:
> One thing my original patch did is to support variable arguments too (IIRC). 
I
> don't think it's worth doing in this first pass, but there are quite a number
of
> APIs that could use this.  e.g., {}.get()

Yeah, I saw that part of the patch. I wanted to get the basic concept in before
taking on variadic functions.

http://codereview.appspot.com/144079/diff/2001/2004
File Objects/methodobject.c (right):

http://codereview.appspot.com/144079/diff/2001/2004#newcode42
Line 42: ml->ml_arity = 1;
On 2009/11/05 18:59:22, nnorwitz wrote:
> These two conditions could be handled as one, but I'm not sure it's worth it. 
I
> don't know if people would find the condensed code more readable:
> 
>          if (ml->ml_flags & (METH_NOARGS|METH_O)) {
>                  ml->ml_flags &= ~(METH_NOARGS|METH_O);
>                  ml->ml_flags |= METH_FIXED;
>                  ml->ml_arity = ((ml->ml_flags & (METH_NOARGS|METH_O)) >> 4) -
> 1;
>          }
> 

I got rid of the METH_NOARGS case by defining it to be METH_FIXED and letting C
fill in ml_arity=0.

http://codereview.appspot.com/144079/diff/2001/2004#newcode113
Line 113: default: abort();
On 2009/11/05 18:08:35, Jeffrey Yasskin wrote:
> Py_BadInternalCall()?

Done.

http://codereview.appspot.com/144079/diff/2001/2004#newcode147
Line 147: to convert them to METH_FIXED and set ml_arity correctly. */
On 2009/11/05 18:59:22, nnorwitz wrote:
> Note this will be reached by any module that hasn't been recompiled and uses
> these flags (which is likely a large number).
> 
> You should make sure to change the module API version number  (in
> Include/modsupport.h) if you didn't.

This is the ABI, not the API, and we've already broken ABI compatibility.
Looking at the past reasons for changing the API version, do we really need to
bump this? I suppose it can't hurt, even though we're not really changing the
API.