Skip to content
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

Directly initialize fmpz from a python int. #64

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

deinst
Copy link
Contributor

@deinst deinst commented Aug 20, 2023

Mimics the code from gmpy2 and so depends on undocumented internals of PyLong.

This uses rather more of the internals of PyLong than may be prudent. This PR is mainly an attempt to see if it works on the various CI instances. If it flies I'll do the fmpz->pylong conversion as well. Also I think we should use _fmpz_promote to get access to the internal mpz of the fmpz, but I have not convinced myself that it is safe.

Mimics the code from gmpy2 and so depends on undocumented internals of
PyLong.
@isuruf
Copy link
Member

isuruf commented Aug 20, 2023

Can you do this with an ifdef on CPython?

@deinst
Copy link
Contributor Author

deinst commented Aug 20, 2023

Can I do what with an ifdef? My knowledge of Cython is not great, and I find the documentation to be of somewhat questionable value. My experience is that Cython does not like #ifdefs, but I would not put much stock in my opinion or experience.

The current implementation works, and using _fmpz_promote seems to work on my machine, but I'm not certain that setting the mpz_t given by _fmpz_promote will always leave the fmpz in a valid state.

@oscarbenjamin
Copy link
Collaborator

Can I do what with an ifdef?

It can be done at the C level in the same way as in C e.g.:

cdef extern from *:
"""
/* FLINT_BITS is not known until C compile time. We need to check if long
* or long long matches FLINT_BITS to know which CPython function to call.
*/
#if FLINT_BITS == 32 && LONG_MAX == 2147483647
#define pylong_as_slong PyLong_AsLongAndOverflow
#elif FLINT_BITS == 64 && LLONG_MAX == 9223372036854775807
#define pylong_as_slong PyLong_AsLongLongAndOverflow
#else
#error FLINT_BITS does not match width of long or long long.
#endif
"""
slong pylong_as_slong(PyObject *pylong, int *overflow)

My knowledge of Cython is not great, and I find the documentation to be of somewhat questionable value

Agreed!

The current implementation works

I presume that @isuruf wants this to work with PyPy also or is considering the general possibility of other Python implementations that support the C API but are not CPython.

@fredrik-johansson
Copy link
Collaborator

_fmpz_promote should be fine if you call _fmpz_demote_val after.

@deinst
Copy link
Contributor Author

deinst commented Aug 21, 2023

@fredrik-johansson Thank you.

No longer allocating a separate fmpz object so should be faster.
This uses the method from sagemath which is a bit simpler than that of
gmpy2.  It is missing a normalization step that is in gmpy2.  I guess
that the normalization step is not necessary.
I'm not entirely sure what is going on, but this seems to be the fastest
set of methods
@deinst
Copy link
Contributor Author

deinst commented Aug 24, 2023

Running some benchmarks it appears that using pylong_as_slong from the old code is slightly faster on my Mac M1 machine.

Here 'hex string' is the current method of writing then reading a hex string, 'Py_SIZE' uses Py_Size to determine the size of the PyLong and using mpz_import if it uses more than one word, and pylong_as_slong checks if the pylong fits in an slong

Note that all of these measurements have about a 4% stddev.

bit size hex string Py_SIZE pylong as slong
2**29 8.93354 8.47803 8.86462
2**30 8.84344 8.61140 8.87248
2**31 9.35676 10.46201 9.37974
2**32 9.49426 11.43341 9.52161
3**33 9.62676 11.87233 9.55271
2**34 9.67202 11.88612 9.58820
2**64 31.29392 14.53042 14.18630
2**128 50.87790 16.64312 17.45658

the benchmark code is

import time
import random
import flint

def testconversion(i, n=10000):
    for j in range(n):
        f = flint.fmpz(i)
        if i != int(f):
            print('hello')

if __name__ == "__main__":
    p = random.randrange(2**29)
    for s in [29,30,31,32,33,34,64,128]:
        r = 2**s
        t0 = time.time()
        for i in range(10000):
            p = random.randrange(r)
            testconversion(p)
        t1 = time.time()

        print('2**{} time is {:.5f}'.format(s, t1-t0))

@oscarbenjamin
Copy link
Collaborator

it appears that using pylong_as_slong from the old code is slightly faster on my Mac M1 machine

These timings are all for relatively small integers. Obviously small integers are an important case but the other important case here is very large integers, like megabyte sized.

@deinst
Copy link
Contributor Author

deinst commented Sep 3, 2023

Not surprisingly all three of the methods scale linearly with the size of the number. Using the same benchmark as before, but using only 100 random numbers converted 100 times, we see that the string conversion method is about 3 times slower asymptotically. The differences between the Py_SIZE and pylong_as_slong methods is well within measurement error for these large numbers.

bit size hex string Py_SIZE pylong as slong
2**1000 0.01371 0.00402 0.00445
2**10000 0.08413 0.03262 0.03167
2**100000 0.75500 0.26258 0.26381
2**1000000 7.57499 2.58292 2.58017
2**10000000 81.53664 26.02679 26.09976

@oscarbenjamin
Copy link
Collaborator

I'm not sure I understand exactly what each of the different benchmarks is timing but it seems that the pylong as slong results are fastest considering both small and large integers. Is that what is currently implemented in this PR?

If it is 3x faster than the current master code for large integers then I think that is well worth it.

Is the only outstanding item having an #ifdef for CPython? I am not sure what preprocessor variable can be used to check for that.

@deinst
Copy link
Contributor Author

deinst commented Sep 4, 2023

Yes, the pylong as slong is what is implemented, and it is three times faster.

@oscarbenjamin
Copy link
Collaborator

Can you do this with an ifdef on CPython?

@isuruf do you know how to do this in C?

I can't see a preprocessor define that could be used to identify CPython specifically. For PyPy you could check for PYPY_VERSION (defined in patchlevel.h).

Not the most straightforward of merges, light editing of flintlib/flint.pxd and
flintlib/fmpz.pxd was needed.
@deinst
Copy link
Contributor Author

deinst commented Sep 23, 2023

Sorry, This kind of fell off my radar.

@oscarbenjamin
Copy link
Collaborator

I think it should be enough to check PYPY_VERSION in an ifdef for this. I am not aware of any other Python implementation that python-flint can be used with besides PyPy and CPython.

@oscarbenjamin
Copy link
Collaborator

There have been some changes to PyLong or at least how it is exposed by the CPython headers in Python 3.12. See aleaxit/gmpy#441

I'm going to rerun CI to see if this passes with 3.12.

@deinst
Copy link
Contributor Author

deinst commented Oct 31, 2023

I will take a look at it. I agree with the sentiment that python integers are a mess.

@skirpichev
Copy link

I hope the PEP 757 will be available with 3.14.

@deinst, are you interested in continuing this work using new API? Here is gmpy2 patch: aleaxit/gmpy#495. Here is API backport for older py3 releases: python/pythoncapi-compat#121.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants