Support PyPy?

[skirpichev]

Performance of the CPyExt going better. Maybe we should add support for PyPy and run basic tests on CI to be sure it's working?

Here is a quick benchmark for multiplication (python -m pyperf timeit -s 'from gmpy2 import fac,mpz;a=fac(mpz(100))' 'a*a'):

Benchmark	ref	ctypes	cffi	gmpy
timeit	781 ns	212 us: 271.14x slower	3.08 us: 3.95x slower	3.81 us: 4.88x slower

On CPython - gmpy2 looks 3.8x faster. It seems using C-API is still slower than CFFI, but not too much.

I think we can do this without too much special cases (see a draft patch below):

1. some defines are missing (not sure why they turned off for PyPy in pythoncapi_compat.h);
2. we should special case mpz <-> int conversions;
3. some C-API functions are missing (PyContextVar_Reset so far: The PyContextVar_Reset() is missing in cpyext pypy/pypy#5081).

On a pros, PyPy people are working hard to make it's support for C-API "correct first". Maybe it helps us catch some bugs. E.g. now without patching the gmpy2_cache.c I got for above benchmark:

RPython traceback:
  File "pypy_module_pypyjit.c", line 945, in portal_11
  File "pypy_interpreter_2.c", line 52468, in handle_bytecode__AccessDirect_None
  File "pypy_interpreter_3.c", line 33415, in dispatch_bytecode__AccessDirect_None
*** Invalid usage of a dying CPython object ***

cpyext, the emulation layer, detected that while it is calling
an object's tp_dealloc, the C code calls back a function that
tries to recreate the PyPy version of the object.  Usually it
means that tp_dealloc calls some general PyXxx() API.  It is
a dangerous and potentially buggy thing to do: even in CPython
the PyXxx() function could, in theory, cause a reference to the
object to be taken and stored somewhere, for an amount of time
exceeding tp_dealloc itself.  Afterwards, the object will be
freed, making that reference point to garbage.
>>> PyPy could contain some workaround to still work if
you are lucky, but it is not done so far; better fix the bug in
the CPython extension.
>>> This object is of type 'gmpy2.mpz'
Error when running timeit benchmark:

Statement:
'a*a'

Is this ok to manage cache in the tp_dealloc as we do?

simple diff to compile and get some things working:

diff --git a/src/gmpy2.h b/src/gmpy2.h
index 9195eeb5..4f862e20 100644
--- a/src/gmpy2.h
+++ b/src/gmpy2.h
@@ -71,6 +71,18 @@ extern "C" {
 #  error "GMPY2 requires Python 3.8 or later."
 #endif
 
+#ifdef PYPY_VERSION
+#  if SIZEOF_VOID_P >= 8
+#    define PyHASH_BITS 61
+#  else
+#    define PyHASH_BITS 31
+#  endif
+# define PyHASH_MULTIPLIER 1000003UL  /* 0xf4243 */
+# define PyHASH_MODULUS (((size_t)1 << _PyHASH_BITS) - 1)
+# define PyHASH_INF 314159
+# define PyHASH_IMAG PyHASH_MULTIPLIER
+#endif
+
 /* Include headers for GMP, MPFR, and MPC. */
 
 #include <gmp.h>
diff --git a/src/gmpy2_cache.c b/src/gmpy2_cache.c
index 93f1f459..3aabb338 100644
--- a/src/gmpy2_cache.c
+++ b/src/gmpy2_cache.c
@@ -179,12 +179,12 @@ GMPy_MPZ_NewInit(PyTypeObject *type, PyObject *args, PyObject *keywds)
 static void
 GMPy_MPZ_Dealloc(MPZ_Object *self)
 {
-   if (global.in_gmpympzcache < CACHE_SIZE &&
+/*   if (global.in_gmpympzcache < CACHE_SIZE &&
        self->z->_mp_alloc <= MAX_CACHE_MPZ_LIMBS) {
         
         global.gmpympzcache[(global.in_gmpympzcache)++] = self;
     }
-    else {
+    else */{
         mpz_clear(self->z);
         PyObject_Free(self);
     }
diff --git a/src/gmpy2_context.c b/src/gmpy2_context.c
index fecd79af..e2912e2c 100644
--- a/src/gmpy2_context.c
+++ b/src/gmpy2_context.c
@@ -184,6 +184,7 @@ GMPy_CTXT_Enter(PyObject *self, PyObject *args)
 static PyObject *
 GMPy_CTXT_Exit(PyObject *self, PyObject *args)
 {
+#ifndef PYPY_VERSION
     CTXT_Object *ctx = (CTXT_Object*)self;
     int res = PyContextVar_Reset(current_context_var, ctx->token);
     Py_DECREF(ctx->token);
@@ -191,6 +192,7 @@ GMPy_CTXT_Exit(PyObject *self, PyObject *args)
         SYSTEM_ERROR("Unexpected failure in restoring context.");
         return NULL;
     }
+#endif
     Py_RETURN_NONE;
 }
 #endif
diff --git a/src/gmpy2_convert_gmp.c b/src/gmpy2_convert_gmp.c
index 63a18e08..4fd4705f 100644
--- a/src/gmpy2_convert_gmp.c
+++ b/src/gmpy2_convert_gmp.c
@@ -42,6 +42,7 @@
 static void
 mpz_set_PyLong(mpz_t z, PyObject *obj)
 {
+#ifndef PYPY_VERSION
     int negative;
     Py_ssize_t len;
     PyLongObject *templong = (PyLongObject*)obj;
@@ -66,6 +67,7 @@ mpz_set_PyLong(mpz_t z, PyObject *obj)
         mpz_neg(z, z);
     }
     return;
+#endif
 }
 
 static MPZ_Object *
@@ -131,7 +133,7 @@ GMPy_PyLong_From_MPZ(MPZ_Object *obj, CTXT_Object *context)
     if (mpz_fits_slong_p(obj->z)) {
         return PyLong_FromLong(mpz_get_si(obj->z));
     }
-
+#ifndef PYPY_VERSION
     /* Assume gmp uses limbs as least as large as the builtin longs do */
 
     size_t count, size = (mpz_sizeinbase(obj->z, 2) +
@@ -151,8 +153,10 @@ GMPy_PyLong_From_MPZ(MPZ_Object *obj, CTXT_Object *context)
         GET_OB_DIGIT(result)[i] = 0;
     }
     _PyLong_SetSignAndDigitCount(result, mpz_sgn(obj->z) < 0, count);
-
     return (PyObject*)result;
+#else
+    Py_RETURN_NONE;
+#endif
 }
 
 static PyObject *

PS: ctypes/cffi toy implementations

• https://github.com/sn6uv/gmpy_cffi
• https://github.com/skirpichev/gmpy_ctypes

[casevh]

I agree with supporting PyPy. I also agree with their analysis of gmpy2_cache.c. The fix looks fine.

[skirpichev]

On Tue, Oct 15, 2024 at 11:07:49PM -0700, casevh wrote: The fix looks fine.

Cerntaily no. I think this kill mpz caching.

[casevh]

Maybe I shouldn't review code when I'm exhausted. 😩 On Tue, Oct 15, 2024 at 11:19 PM Sergey B Kirpichev < ***@***.***> wrote:

…

On Tue, Oct 15, 2024 at 11:07:49PM -0700, casevh wrote: > The fix looks fine. Cerntaily no. I think this kill mpz caching. — Reply to this email directly, view it on GitHub <#523 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAMR23ZDRRMMZZ4E4IUGMODZ3YANJAVCNFSM6AAAAABQASNIROVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMJVHAZDONRVGM> . You are receiving this because you commented.Message ID: ***@***.***>