-
Notifications
You must be signed in to change notification settings - Fork 86
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update int <-> mpz conversion #484
Conversation
It is a definite performance improvement for the conversion of small mpz->int. There is a slight performance decrease for the conversion of int->mpz. I used the the PyLong_AsLongAndOverflow() to avoid creation of a temporary mpz variable. It wasn't used to for a faster conversion to mpz. I would revert the changes for int->mpz. I need to think more about the Windows platform. The new compact integer object in 3.13 is a restricted range ssize_t value. In WIndows, ssize_t is equivalent to long long so we can't use mpz_fits_si. There is PyLong_FromSsize_t. What is the best way to extract a ssize_t from an mpz type? I want to allow Python 3.13+ to construct compact values whenever possible. Using PyLong_From[Long|Ssize_t] should be a stable way to do it. |
Done. I'll not try other variants here to not block release.
I doubt there are other options. This will solve mentioned issue with small integers (previously, new objects were created). |
Can you double-check my logic to check if an mpz object can be converted to a 64-bit ssize_t assuming a 64-bit unsigned limb? if mpz_size(object) > 1 then return false An implementation of GMPy_mpz_AsSssize_tAndOverflow() using the above logic followed by PyLong_FromSsize_t() should allow Windows to create compact integers for all the same values as Linux/MacOS. I don't care if B1 is released without this but I would like to add this for an RC1 release. |
Looks fine, assuming given fixed sizes. But I'm not sure I realize what you are trying to solve. Current PR do mpz->int conversion in a portable way for small mpz's (mpz_fits_slong_p(), mpz_get_si() and PyLong_FromLong()) and ensure that int's value is taken from the cache (i.e. by get_small_int()). That solves For the rest - we fallback to old code. I think if we want to streamline conversion for bigger values - this will require a better API both from the CPython and GMP. |
The current patch solves two issues: (1) small ints were not used and (2) on platforms where sizeof(long) == sizeof(ssize_t) a compact value would be returned for all possible valid compact results (since they all fit in a long). But on Windows, sizeof(long) != sizeof(ssize_t) so values outside the range of a long but inside the range of a compact are converted to PyLong instead. For example, int(mpz(2**33)) will be a compact type on Linux but a PyLong type on Windows. I'm concerned that the different types will trigger a slower code path on Windows. I'll commit your PR as-is. I've already started on a version of GMPy_MPZ_AsSsize_tAndOverflow and I'll commit that when it is tested. |
Hmm, are you sure (you can test by
|
BTW, >>> example.is_compact(2**33)
False
>>> example.is_compact(2**29) # 30 bits per digit
True
>>> example.is_compact(int(gmpy2.mpz(2**30)))
False
>>> example.is_compact(int(gmpy2.mpz(2**29)))
True static PyObject *
is_compact(PyObject *self, PyObject *op)
{
return PyBool_FromLong(PyUnstable_Long_IsCompact((PyLongObject *) op));
} |
Sigh. I think I remember reading early proposals where compact values had a range of 60 bits. And the documentation states that compact values will not require more that 63 bits on systems with a 64-bit ssize_t. And that tagged pointers would be used. So either I'm not interpreting the code correctly or my memory is faulty. Or both. ;) If I publish 2.2.0b1 with binary wheels and just a source release of 2.1.6, will "pip install gmpy2" attempt a source installation of 2.1.6 over a 2.2.0.b1 binary wheel? If so, does bumping to 2.2.0rc1 allow "pip install gmpy2" to choose the 2.2.0 binary wheels? |
Sure. pip will prefer stable release, unless you use
No. rc1 is still pre-release version. |
@casevh, that solves issue, mentioned in #483. But this code should be benchmarked.