Skip to content

Commit e427a56

Browse files
author
Release Manager
committed
gh-41171: Speedup and proper fix for gap conversion from sage/python integer follow up to #41057 My machine has `sizeof(mp_limb_t) == sizeof(UInt)` true, but I've tested both branches by changing the `if ...` to `if False` and see that the tests pass. the expression `num_gmp_limbs * sizeof(mp_limb_t)` cannot overflow (because there's a buffer in memory with that size), so as long as `sizeof(mp_limb_t) > 1` (which must be the case, see below), `-num_gmp_limbs` cannot overflow `UInt`. ## note on word size From `gmp-h.in`: ``` #ifdef __GMP_SHORT_LIMB typedef unsigned int mp_limb_t; typedef int mp_limb_signed_t; #else #ifdef _LONG_LONG_LIMB typedef unsigned long long int mp_limb_t; typedef long long int mp_limb_signed_t; #else typedef unsigned long int mp_limb_t; typedef long int mp_limb_signed_t; #endif #endif typedef unsigned long int mp_bitcnt_t; /* For reference, note that the name __mpz_struct gets into C++ mangled function names, which means although the "__" suggests an internal, we must leave this name for binary compatibility. */ typedef struct { int _mp_alloc; /* Number of *limbs* allocated and pointed to by the _mp_d field. */ int _mp_size; /* abs(_mp_size) is the number of limbs the last field points to. If _mp_size is negative this is a negative number. */ mp_limb_t *_mp_d; /* Pointer to the limbs. */ } __mpz_struct; ``` From `gap/src/common.h`: ``` typedef intptr_t Int; typedef uintptr_t UInt; ``` so their sizes need not always be equal. ## note on gap internal implementation of integer That said, `gap/src/integer.c` contains this ``` GAP_STATIC_ASSERT( sizeof(mp_limb_t) == sizeof(UInt), "gmp limb size incompatible with GAP word size"); ``` maybe we're safe...? On the other hand it's entirely possible that gap and sage are compared with different settings of gmp, they're distinct shared libraries. also there's this ``` /*********************************************************************** ***** ** ** 'ADDR_INT' returns a pointer to the limbs of the large integer 'obj'. ** 'CONST_ADDR_INT' does the same, but returns a const pointer. */ EXPORT_INLINE UInt * ADDR_INT(Obj obj) { GAP_ASSERT(IS_LARGEINT(obj)); return (UInt *)ADDR_OBJ(obj); } static Obj GMPorINTOBJ_MPZ( mpz_t v ) { return MakeObjInt((const UInt *)v->_mp_d, v->_mp_size); } ``` the former is actually exported. If we're fine with using more internals, we could call `NewBag` directly with the correct size, but I think that's unnecessary. ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [ ] The title is concise and informative. - [ ] The description explains in detail what this PR is about. - [ ] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - #12345: short description why this is a dependency --> <!-- - #34567: ... --> URL: #41171 Reported by: user202729 Reviewer(s): Chenxin Zhong, Tobias Diez, user202729
2 parents e23886d + 48e92fc commit e427a56

File tree

3 files changed

+75
-117
lines changed

3 files changed

+75
-117
lines changed

src/sage/libs/gap/element.pxd

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ from sage.structure.element cimport Element, ModuleElement, RingElement
1515
cdef Obj make_gap_list(sage_list) except NULL
1616
cdef Obj make_gap_matrix(sage_list, gap_ring) except NULL
1717
cdef Obj make_gap_record(sage_dict) except NULL
18-
cdef Obj make_gap_integer(sage_int) except NULL
18+
cdef Obj make_gap_integer(x) except NULL
1919
cdef Obj make_gap_string(sage_string) except NULL
2020

2121
cdef GapElement make_any_gap_element(parent, Obj obj)

src/sage/libs/gap/element.pyx

Lines changed: 61 additions & 116 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ elements. For general information about GAP, you should read the
1616
# ****************************************************************************
1717

1818
from cpython.object cimport Py_EQ, Py_NE, Py_LE, Py_GE, Py_LT, Py_GT
19-
from cpython.mem cimport PyMem_Malloc, PyMem_Free
19+
from libc.stdlib cimport free
2020

2121
from sage.libs.gap.gap_includes cimport *
2222
from sage.libs.gap.libgap import libgap
@@ -25,6 +25,7 @@ from sage.libs.gap.util import GAPError, gap_sig_on, gap_sig_off
2525
from sage.libs.gmp.mpz cimport *
2626
from sage.libs.gmp.pylong cimport mpz_get_pylong, mpz_set_pylong
2727
from sage.cpython.string cimport str_to_bytes, char_to_str
28+
from sage.rings.integer cimport Integer
2829
from sage.rings.integer_ring import ZZ
2930
from sage.rings.rational_field import QQ
3031
from sage.rings.real_double import RDF
@@ -213,135 +214,79 @@ cdef Obj make_gap_record(sage_dict) except NULL:
213214
GAP_Leave()
214215

215216

216-
cdef Obj make_gap_integer(sage_int) except NULL:
217+
cdef extern from *:
218+
long __BYTE_ORDER__, __ORDER_LITTLE_ENDIAN__
219+
220+
221+
cdef Obj make_gap_integer_from_mpz(mpz_srcptr z) except NULL:
222+
"""
223+
Internal helper to convert ``mpz`` integer into ``Gap`` integer.
224+
"""
225+
cdef size_t num_gmp_limbs = mpz_size(z)
226+
cdef void* temp
227+
cdef size_t num_gap_words
228+
if sizeof(mp_limb_t) == sizeof(UInt) or (
229+
__BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__ and sizeof(mp_limb_t) * num_gmp_limbs % sizeof(UInt) == 0):
230+
# use GMP internal to avoid memory allocation
231+
num_gap_words = num_gmp_limbs * sizeof(mp_limb_t) / sizeof(UInt) if sizeof(mp_limb_t) != sizeof(UInt) else num_gmp_limbs
232+
try:
233+
GAP_Enter()
234+
return GAP_MakeObjInt(<const UInt*>z._mp_d, -num_gap_words if mpz_sgn(z) < 0 else num_gap_words)
235+
finally:
236+
GAP_Leave()
237+
else:
238+
temp = mpz_export(NULL, &num_gap_words, -1, sizeof(UInt), 0, 0, z) # because of sage.ext.memory, this uses sage_sig_malloc
239+
try:
240+
GAP_Enter()
241+
return GAP_MakeObjInt(<const UInt*>temp, -num_gap_words if mpz_sgn(z) < 0 else num_gap_words)
242+
finally:
243+
GAP_Leave()
244+
free(temp)
245+
246+
247+
def make_GapElement_Integer_from_sage_integer(parent, Integer x):
248+
"""
249+
Internal helper to convert Sage :class:`~sage.rings.integer.Integer` into GapElement objects.
250+
Not to be used directly, use ``libgap(x)`` instead.
251+
252+
TESTS::
253+
254+
sage: for x in [0, 1, 2**31, 2**32, 2**63, 2**64, 2**128]:
255+
....: for y in [x, -x, x-1]:
256+
....: assert str(libgap(y)) == str(y), y
257+
258+
Check that the following is fast (i.e. no conversion to decimal is performed)::
259+
260+
sage: ignore = libgap(1<<500000000)
217261
"""
218-
Convert Sage integer or Python integer into Gap integer
262+
return make_GapElement_Integer(parent, make_gap_integer_from_mpz(x.value))
263+
264+
265+
cdef Obj make_gap_integer(x) except NULL:
266+
"""
267+
Convert Python integer into Gap integer. Not to be used directly, use ``libgap(x)`` instead.
219268
220269
INPUT:
221270
222-
- ``sage_int`` -- Sage integer or Python int
271+
- ``x`` -- Python ``int`` object
223272
224273
OUTPUT: the integer as a GAP ``Obj``
225274
226-
TESTS:
275+
TESTS::
227276
228-
Test with Sage integers::
277+
sage: for x in [0, 1, 2**31, 2**32, 2**63, 2**64, 2**128]:
278+
....: for y in [x, -x, x-1]:
279+
....: assert str(libgap(int(y))) == str(y), y
229280
230-
sage: libgap(0) # indirect doctest
231-
0
232-
sage: libgap(1)
233-
1
234-
sage: libgap(-1)
235-
-1
236-
sage: libgap(2**31)
237-
2147483648
238-
sage: libgap(-2**63)
239-
-9223372036854775808
240-
sage: libgap(2**64)
241-
18446744073709551616
242-
sage: libgap(-(2**256))
243-
-115792089237316195423570985008687907853269984665640564039457584007913129639936
244-
245-
Test with Python int (not Sage Integer)::
246-
247-
sage: libgap(int(0))
248-
0
249-
sage: libgap(int(1))
250-
1
251-
sage: libgap(int(-1))
252-
-1
253-
sage: libgap(int(10**30))
254-
1000000000000000000000000000000
255-
sage: libgap(-int(10**30))
256-
-1000000000000000000000000000000
257-
sage: libgap(int(2**100))
258-
1267650600228229401496703205376
259-
260-
Test round-trip conversion::
261-
262-
sage: n = int(123456789012345678901234567890)
263-
sage: gap_n = libgap(n)
264-
sage: gap_n.sage() == n
265-
True
281+
Check that the following is fast (i.e. no conversion to decimal is performed)::
266282
267-
sage: n = factorial(100)
268-
sage: gap_n = libgap(n)
269-
sage: gap_n.sage() == n
270-
True
283+
sage: ignore = libgap(int(1<<500000000))
271284
"""
272285
cdef mpz_t temp
273-
cdef Obj result
274-
cdef Int size
275-
cdef Int sign
276-
cdef UInt* limbs = NULL
277-
cdef size_t limb_count
278-
cdef size_t i
279-
280-
# We need to handle this carefully to avoid accessing GMP internals
281286
mpz_init(temp)
282287
try:
283-
# Convert Python int to GMP mpz_t
284-
mpz_set_pylong(temp, sage_int)
285-
286-
# Handle zero specially (mpz_size returns 0 for zero)
287-
size = mpz_size(temp)
288-
if size == 0:
289-
return GAP_NewObjIntFromInt(0)
290-
291-
# Get the sign: mpz_sgn returns -1, 0, or 1
292-
sign = <Int>mpz_sgn(temp)
293-
294-
# Allocate limb buffer for export
295-
# sizeof(mp_limb_t) may differ from sizeof(UInt), so we need to handle this
296-
if sizeof(mp_limb_t) == sizeof(UInt):
297-
# Direct case: limb sizes match, we can use mpz_export directly
298-
# into a UInt buffer
299-
limbs = <UInt*>PyMem_Malloc(size * sizeof(UInt))
300-
if limbs == NULL:
301-
raise MemoryError("Failed to allocate limb buffer")
302-
303-
# Export limbs: order=-1 (least significant first, native GMP/GAP order),
304-
# size=sizeof(UInt), endian=0 (native), nails=0 (use full limbs)
305-
mpz_export(limbs, &limb_count, -1, sizeof(UInt), 0, 0, temp)
306-
307-
# GAP_MakeObjInt expects signed size (negative for negative numbers)
308-
if sign < 0:
309-
size = -<Int>limb_count
310-
else:
311-
size = <Int>limb_count
312-
313-
try:
314-
GAP_Enter()
315-
result = GAP_MakeObjInt(<const UInt*>limbs, size)
316-
finally:
317-
GAP_Leave()
318-
PyMem_Free(limbs)
319-
320-
return result
321-
else:
322-
# Fallback case: limb sizes don't match
323-
# We need to copy limb-by-limb using mpz_getlimbn
324-
# This is slower but portable
325-
limbs = <UInt*>PyMem_Malloc(size * sizeof(UInt))
326-
if limbs == NULL:
327-
raise MemoryError("Failed to allocate limb buffer")
328-
329-
# Copy each limb individually
330-
for i in range(size):
331-
limbs[i] = <UInt>mpz_getlimbn(temp, i)
332-
333-
# GAP_MakeObjInt expects signed size
334-
if sign < 0:
335-
size = -size
336-
337-
try:
338-
GAP_Enter()
339-
result = GAP_MakeObjInt(<const UInt*>limbs, size)
340-
finally:
341-
GAP_Leave()
342-
PyMem_Free(limbs)
343-
344-
return result
288+
mpz_set_pylong(temp, x)
289+
return make_gap_integer_from_mpz(temp)
345290
finally:
346291
mpz_clear(temp)
347292

src/sage/rings/integer.pyx

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6365,6 +6365,19 @@ cdef class Integer(sage.structure.element.EuclideanDomainElement):
63656365
"""
63666366
return str(self)
63676367

6368+
def _libgap_(self):
6369+
"""
6370+
Convert this integer to ``libgap``. Not to be used directly, use ``libgap(x)``.
6371+
6372+
EXAMPLES::
6373+
6374+
sage: libgap(1)
6375+
1
6376+
"""
6377+
from sage.libs.gap.element import make_GapElement_Integer_from_sage_integer # avoid compile-time dependency
6378+
from sage.libs.gap.libgap import libgap
6379+
return make_GapElement_Integer_from_sage_integer(libgap, self)
6380+
63686381
@property
63696382
def __array_interface__(self):
63706383
"""

0 commit comments

Comments
 (0)