Opened 10 years ago

Closed 9 years ago

#14817 closed enhancement (fixed)

Inefficiency in copying PARI objects to the heap

Reported by: Peter Bruin Owned by: tbd
Priority: minor Milestone: sage-5.12
Component: performance Keywords:
Cc: Jean-Pierre Flori Merged in: sage-5.12.beta0
Authors: Peter Bruin Reviewers: Volker Braun
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by Peter Bruin)

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: trac14817-copy_pari_objects.patch

Attachments (1)

trac14817-copy_pari_objects.patch (1.8 KB) - added by Peter Bruin 10 years ago.
based on 5.11.beta3

Download all attachments as: .zip

Change History (9)

Changed 10 years ago by Peter Bruin

based on 5.11.beta3

comment:1 Changed 10 years ago by Peter Bruin

Description: modified (diff)
Status: newneeds_review

comment:2 Changed 10 years ago by Jean-Pierre Flori

Cc: Jean-Pierre Flori added

comment:3 Changed 10 years ago by Jean-Pierre Flori

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).

comment:4 Changed 10 years ago by Jean-Pierre Flori

(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.)

comment:5 Changed 10 years ago by Volker Braun

Reviewers: Volker Braun
Status: needs_reviewpositive_review

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.

comment:6 Changed 10 years ago by Peter Bruin

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.

comment:7 Changed 10 years ago by Jean-Pierre Flori

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

comment:8 Changed 9 years ago by Jeroen Demeyer

Merged in: sage-5.12.beta0
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.