Inefficiency in copying PARI objects to the heap

[pjbruin]

When PARI objects are wrapped into Python objects, they are copied from the PARI stack to a malloc'ed memory block. This is done by the function deepcopy_to_python_heap() in sage/libs/pari/gen.pyx. To determine how much memory to allocate, this function creates a dummy copy on the stack and measures by how much the stack pointer goes down. This is inefficient and can be improved by calling PARI's gsizebyte() function, which does exactly what we want.

Also, copying the object to the malloc'ed block is done inelegantly by temporarily changing the global variables top, bot and avma defining the PARI stack. Again, there is a PARI function that does what we want: gcopy_avma() does the same as gcopy(), but copies to a user-specified address.

I am attaching a patch that replaces deepcopy_to_python_heap() by a simpler and faster implementation. (It also deletes a comment that seems to refer to code that no longer exists.)

Apply: attachment: trac14817-copy_pari_objects.patch

CC: @jpflori

Component: performance

Author: Peter Bruin

Reviewer: Volker Braun

Merged: sage-5.12.beta0

Issue created by migration from https://trac.sagemath.org/ticket/14817

[pjbruin]

Attachment: trac14817-copy_pari_objects.patch.gz

based on 5.11.beta3

[pjbruin]

Description changed:

--- 
+++ 
@@ -3,3 +3,5 @@
 Also, copying the object to the `malloc`'ed block is done inelegantly by temporarily changing the global variables `top`, `bot` and `avma` defining the PARI stack.  Again, there is a PARI function that does what we want: `gcopy_avma()` does the same as `gcopy()`, but copies to a user-specified address.
 
 I am attaching a patch that replaces `deepcopy_to_python_heap()` by a simpler and faster implementation.  (It also deletes a comment that seems to refer to code that no longer exists.)
+
+Apply: [attachment: trac14817-copy_pari_objects.patch](https://github.com/sagemath/sage-prod/files/10658044/trac14817-copy_pari_objects.patch.gz)

[jpflori]

comment:3

The patch looks and applies fine.

Could you provide some timing here demonstrating the speed up?
It would be nice to keep track of it at least here (obviously theres no way to provide doctests for that).

[jpflori]

comment:4

(For later tickets: I don't think you have to mention the ticket number in the commit message because Jeroen's script automatically add them.)

[vbraun]

comment:5

I don't really see why we need to further justify using the built-in function instead of hacking our own. With a useless stack copy to boot. Its obviously better.

[vbraun]

Reviewer: Volker Braun

[pjbruin]

comment:6

My main motivation was to make this function do The Right Thing(tm) instead of the ugly hack that is currently there. There is actually a speed-up, though; I did some unscientific tests and came to the conclusion that it is negligible for very small PARI objects (like integers), around 10-15% for (small) number fields or elliptic curves, and 25% for a 100 x 100 matrix over the integers.

[jpflori]

comment:7

Great, anyway, I agree with Volker's move and your first justification: it's the right thing to do.