sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.37k stars 462 forks source link

fix Memory Leak in libsingular's reduce() #4380

Closed simon-king-jena closed 15 years ago

simon-king-jena commented 15 years ago

At http://groups.google.com/group/sage-support/browse_thread/thread/b997f95c1e2503e0/5db5f9e7d4c8faf2 was discussion about a memory leak. I found a reasonably short bit of code that triggers the leak.

In test.pyx:

from sage.all import copy

def Test(I):
    R=I.ring()
    p=R.random_element()
    J0=list(I.reduced_basis())
    while(1):
        J = copy(J0)
        for i in range(100):
            q=p.reduce(J)
            J.append(q)

Apparently the memory consumption should not grow, since J returns to its original state after finishing the for loop. However, the following Sage session is leaking (i.e., the memory consumption grows rapidly, and after a few seconds 1GB are filled up):

sage: attach test.pyx
Compiling /home/king/Projekte/f5/test.pyx...
sage: from sage.rings.ideal import Cyclic
sage: R=PolynomialRing(QQ,'x',5)
sage: I=Cyclic(R).homogenize()
sage: Test(I)

The while loop comprises

  1. copying of a Sequence (J0) of MPolynomial_libsingular
  2. reduction of MPolynomial_libsingular
  3. appending to a Sequence.

In one of these three steps must be the leak. I suspect it is the reduction and will try to verify it.

Component: commutative algebra

Keywords: memory leak libsingular

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

simon-king-jena commented 15 years ago
comment:1

Sorry, it is not copying a Sequence but a list. But that shouldn't matter.

simon-king-jena commented 15 years ago
comment:2

... and the line J.append(q) (that would occur, e.g., in a Groebner basis computation) is not needed either. Replacing Test(I) by the following code triggers the leak as well:

def Test(I):
    R=I.ring()
    p=R.random_element()
    J=list(I.reduced_basis())
    while(1):
        q=p.reduce(J)
simon-king-jena commented 15 years ago
comment:3

The patch libsingularLeak.patch partially solves the problem.

I observed that when a coercion error occurs an intermediately generated ideal _I is explicitly destroyed with id_Delete(&_I,r). But when there is no error and the result is returned, then _I is not deleted. But I think it should.

With the patch, the above Test(I) still shows an increase of memory, but a rather moderate increase. So, it seems to be a partial solution of the problem, but more needs to be done.

85eec1a4-3d04-4b4d-b711-d4db03337c41 commented 15 years ago
comment:4

Simon,

nice work. If you find any more issues please open a new ticket, so that this patch can be merged. I will review this patch shortly.

Cheers,

Michael

simon-king-jena commented 15 years ago

Attachment: libsingularLeak.patch.gz

Fixing the leak, after a suggestion of malb

simon-king-jena commented 15 years ago
comment:5

Since the announced review of Michael didn't come yet, I hope it is ok that I changed the patch according to a suggestion of malb, without opening a new ticket.

The new patch seems to fix it completely.

85eec1a4-3d04-4b4d-b711-d4db03337c41 commented 15 years ago
comment:6

Replying to @simon-king-jena:

Since the announced review of Michael didn't come yet, I hope it is ok that I changed the patch according to a suggestion of malb, without opening a new ticket.

Yep, I am testing that patch now.

The new patch seems to fix it completely.

:)

Cheers,

Michael

85eec1a4-3d04-4b4d-b711-d4db03337c41 commented 15 years ago
comment:7

This patch also seems to fix the problem with sr.py consuming vast amounts of memory, but I will do some independent testing first.

Cheers,

Michael

85eec1a4-3d04-4b4d-b711-d4db03337c41 commented 15 years ago
comment:8

Replying to @sagetrac-mabshoff:

This patch also seems to fix the problem with sr.py consuming vast amounts of memory, but I will do some independent testing first.

Unfortunately it doesn't fix the segfault (see #3758), but the amount of memory allocated goes down significantly already.

Cheers,

Michael

Cheers,

Michael

85eec1a4-3d04-4b4d-b711-d4db03337c41 commented 15 years ago
comment:9

Valgrinds clean, passes doctests, positive review.

Cheers,

Michael

85eec1a4-3d04-4b4d-b711-d4db03337c41 commented 15 years ago
comment:10

Merged in Sage 3.2alpha2