#18868: a MemoryAllocator object for easier Cython memory management
-------------------------+-------------------------------------------------
Reporter: | Owner:
ncohen | Status: needs_review
Type: | Milestone: sage-6.8
enhancement | Resolution:
Priority: major | Merged in:
Component: | Reviewers:
cython | Work issues:
Keywords: | Commit:
Authors: | 9597eec5a42d72f66d56245f7312af3d69bbbf13
Nathann Cohen | Stopgaps:
Report Upstream: N/A |
Branch: |
public/18868 |
Dependencies: |
-------------------------+-------------------------------------------------
Changes (by ncohen):
* status: needs_work => needs_review
Comment:
> This {{{include 'sage/ext/interrupt.pxi'}}} should be in the .pyx file.
Done
> For performance, replace
> {{{cdef MemoryAllocator mem = MemoryAllocator()}}}
> by
> {{{cdef MemoryAllocator mem =
<MemoryAllocator>MemoryAllocator.__new__(MemoryAllocator)}}}
Makes no difference in any of the functions I touched, so I did not do it.
Less
readable.
> Can you please support all memory-allocation functions, like calloc,
realloc,
> (re)allocarray and replace the dangerous(*) calls like
I added support for calloc. I do not need realloc (you can add a commit if
you
need it).
About realloc:
> I guess that realloc(array) might not be so easy to support, so you can
forget
> about that.
+1
> But I would certainly support allocarray
Add a commit if you like. My problem with 'allocarray' is that it is
Sage's
terminology (as far as I know), and that I prefer to stick to more widely
understood terms like 'malloc/calloc'. I was just saying to David that we
may
eventually move some C graph code away from sage and make it an
independent C
library.
> And I don't understand why you use
>
> {{{
> cdef void * val = malloc(size)
> if val == NULL:
> raise MemoryError()
> }}}
> instead of
> {{{
> cdef void * val = check_malloc(size)
> }}}
Done
> [1] recommends PyMem_Malloc, PyMem_Realloc, PyMem_Free over the system
> functions. Why do you stick with malloc etc?
Personally, for a simple reason: I trust C functions. Officially, it is
for the
reason Jeroen mentionned. Note that we can satisfy everybody: it is
possible to
have `sage_malloc` (which behaves properly with respect to interrupts)
call your
functions instead of C ones.
> More about performance: note that your use of MemoryAllocator needs at
least 2
> extra memory allocation calls compared to not using MemoryAllocator
This is not a problem for the codes I touched, for malloc is never a
dominant
cost in any of them. You can add a commit if you like, though please keep
the
code simple and understandable if you do.
> More for performance: replace
> {{{cdef enlarge_if_needed(self):}}}
> by
> {{{cdef int enlarge_if_needed(self) except -1:}}}
Done.
> One more detail: replace {{{int}}} with {{{size_t}}}
Done.
Jeroen, could you send reviews as one big comment rather than adding one
every
time you notice something? Everybody in Cc receives an email for each
comments,
and it is also a bit harder to address your points:
- One never knows if you are done reading the code or if we can expect a
new
comment in the next seconds
- Answering your points like I do now is a bit harder: instead of clicking
on
'reply' (to your comment), I copy/paste all your individual messages
into a
text file, then start answering them.
Thaaaaaaaaanks,
Nathann
--
Ticket URL: <http://trac.sagemath.org/ticket/18868#comment:15>
Sage <http://www.sagemath.org>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica,
and MATLAB
--
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
Visit this group at http://groups.google.com/group/sage-trac.
For more options, visit https://groups.google.com/d/optout.
