Author mark.dickinson
Recipients belopolsky, draghuram, mark.dickinson, rhettinger, stutzbach
Date 2010-05-12.17:59:02
SpamBayes Score 0.000763917
Marked as misclassified No
Message-id <1273687145.76.0.970541386666.issue8692@psf.upfronthosting.co.za>
In-reply-to
Content
Now that I've looked at the patch properly:

I'm +1 on including some version of this patch.  At the time that the original math.factorial went in (see issue 2138) we were hurrying a bit to get it in before the first 2.6 beta, so ended up with a simple implementation, with the understanding (I think) that it could be improved later.

It looks like you and Alexander are doing a great job of hammering out the fine detail;  I only have a few comments at this stage.  I predict that you're not going to like the first one ;-).  The others are just technical issues.

(1) In the interests of simplicity, please could we lose the 'long' optimization in factorial_part_product?  That is, get rid of the if (m == n+2) branch, and just let that case recurse normally---which means that we end up calling PyNumber_Multiply in some cases instead of doing a C long by C long multiplication.  Then we can get rid of multiply_cutoff entirely.  I'm +1 on the improved algorithm, and I realize that the optimization does have an effect (some unscientific tests showed me a 18% speed increase in typical cases) but for me this optimization goes past the simplicity/speed tradeoff.  There's always the option of adding something like this back in later, once the new algorithm's gone in.

(2) You're missing a Py_DECREF(part) in factorial_loop, so factorial(n) leaks references (for n > 12).

(3) The line "k = (n + m) / 2;" in factorial_part_product invokes undefined behaviour (from signed overflow) if n and m are large.  We're not going to get meaningful results in this case anyway, but UB should be avoided if at all possible.  Perhaps rewrite this as "k = n + (m - n) / 2;"?

(4) And please do restore the PyLong_FromDouble line in the main routine, rather than using a C double-to-long cast.  The direct conversion again leads to undefined behaviour for large doubles (cf. C99 6.3.1.4,p2).
History
Date User Action Args
2010-05-12 17:59:05mark.dickinsonsetrecipients: + mark.dickinson, rhettinger, belopolsky, draghuram, stutzbach
2010-05-12 17:59:05mark.dickinsonsetmessageid: <1273687145.76.0.970541386666.issue8692@psf.upfronthosting.co.za>
2010-05-12 17:59:03mark.dickinsonlinkissue8692 messages
2010-05-12 17:59:02mark.dickinsoncreate