search

sagemath / sage

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

Memory leaks in libsingular polynomial evaluation and substitution #27261

Open simon-king-jena opened 5 years ago

simon-king-jena commented 5 years ago

The following two examples leak memory (in sagemath from 8.9 to 9.3.rc5)

sage: R = PolynomialRing(ZZ, 'x', 50)
sage: d = {str(g): g for g in R.gens()}
sage: p = sum(d.values())
sage: import gc
sage: mem0 = get_memory_usage()
sage: for i in range(20):
....:     for _ in range(50):
....:         _ = p.subs(**d)
....:     _ = gc.collect()
....:     mem1 = get_memory_usage()
....:     if mem1 > mem0:
....:         print(i, mem1-mem0)
....:         mem0 = mem1

and

sage: R.<x, y> = ZZ[]
sage: p = (x + y)**100
sage: mem0 = get_memory_usage()
sage: for i in range(20):
....:     _ = p(x + y, y)
....:     _ = gc.collect()
....:     mem1 = get_memory_usage()
....:     if mem1 > mem0:
....:         print(i, mem1-mem0)

This was reported on sage-devel and ask.sagemath.org.

We fix .subs (the first example above) by modifying the appropriate singular call. We identified two possibly related memory leaks in singular

In the mean time, we use the non-satisfactory solution of going via naive substitution in Python. To do so, we moved the generic implementation on the base class MPolynomial.

See also #13447 also related to memory handling in singular.

Upstream: Reported upstream. No feedback yet.

CC: @nbruin @malb

Component: memleak

Keywords: libsingular polynomial memleak

Author: Vincent Delecroix, ​Dima Pasechnik

Branch/Commit: u/vdelecroix/27261 @ edf8847

Reviewer: Dima Pasechnik

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

videlec commented 3 years ago
comment:37

Replying to @videlec:

Sage allows some weird things

sage: R.<x,y> = ZZ[]
sage: S.<q> = ZZ[]
sage: (x+y).subs(x=q)  # expected
Traceback (most recent call last):
...
TypeError: unsupported operand parent(s) for +:
  'Univariate Polynomial Ring in q over Integer Ring' and
  'Multivariate Polynomial Ring in x, y over Integer Ring'
sage: x.subs(x=q)  # why does it work!?
q

and even worse

sage: x(q, y)
q
videlec commented 3 years ago
comment:38

Replying to @videlec:

Replying to @videlec:

Sage allows some weird things

sage: R.<x,y> = ZZ[]
sage: S.<q> = ZZ[]
sage: (x+y).subs(x=q)  # expected
Traceback (most recent call last):
...
TypeError: unsupported operand parent(s) for +:
  'Univariate Polynomial Ring in q over Integer Ring' and
  'Multivariate Polynomial Ring in x, y over Integer Ring'
sage: x.subs(x=q)  # why does it work!?
q

and even worse

sage: x(q, y)
q

https://groups.google.com/g/sage-devel/c/vGzNJKAQWbs

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

01ea16f27261: fix memory leak in polynomial substitution
2f9a21827261: doctest subs and `__call__` leak
26f694727261: fix misformed doctests
796e85427261: fix modforms
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Changed commit from f90202c to 796e854

videlec commented 3 years ago
comment:40

Rebased on 9.3.

I implemented the proposal of Nils Bruin from the sage-devel thread to implement the generic __call__. The behaviour is doctested but it would even be better if it was part of the TestSuite (see #31668). The doctest fixes from 796e854 (modular forms on Hecke triangle groups) look a bit weird. But at least the output is still correct.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

ed83ed327261: more robust doctest in polynomial.pyx
394d17327261: fix error message in __call
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Changed commit from 796e854 to 394d173

videlec commented 3 years ago

Description changed:

--- 
+++ 
@@ -31,6 +31,6 @@

 This was reported [on sage-devel](https://groups.google.com/forum/#!topic/sage-devel/iO1SzoW0kcw) and [ask.sagemath.org](https://ask.sagemath.org/question/29444/high-memory-usage-when-substituting-variables/).

-We fix `.subs` (the first example above) by modifying the appropriate singular call. For `__call__`, we identified a memory leak in singular, see [singular issue #1089](https://github.com/Singular/Singular/issues/1089). In the mean time, we use the non-satisfactory solution of going via naive substitution in Python.
+We fix `.subs` (the first example above) by modifying the appropriate singular call. For `__call__`, we identified a memory leak in singular, see [singular issue #1089](https://github.com/Singular/Singular/issues/1089). In the mean time, we use the non-satisfactory solution of going via naive substitution in Python. To do so, we moved the generic implementation on the base class `MPolynomial`.

 See also #13447 also related to memory handling in singular.
dimpase commented 3 years ago
comment:43

testing https://github.com/Singular/Singular/issues/1089#issuecomment-844015795 now

videlec commented 3 years ago
comment:44

Replying to @dimpase:

testing https://github.com/Singular/Singular/issues/1089#issuecomment-844015795 now

Good idea. Thanks.

videlec commented 3 years ago

Changed upstream from Reported upstream. No feedback yet. to Fixed upstream, but not in a stable release.

dimpase commented 3 years ago
comment:46

Unfortunately, this Singular patch only fixes the reported in https://github.com/Singular/Singular/issues/1089 problem, but not the leaks here.

dimpase commented 3 years ago

Changed upstream from Fixed upstream, but not in a stable release. to Reported upstream. No feedback yet.

videlec commented 3 years ago
comment:47

Replying to @dimpase:

Unfortunately, this Singular patch only fixes the reported in https://github.com/Singular/Singular/issues/1089 problem, but not the leaks here.

It would be nice to track the underlying singular issue. However, this should not hold the ticket any longer.

dimpase commented 3 years ago
comment:48

are the leaks in the ticket text now fixed?

videlec commented 3 years ago
comment:49

Replying to @dimpase:

are the leaks in the ticket text now fixed?

Both of them as explained in the ticket description (subs thanks to you and __call__ via naive symbolic evaluation). Also they are both doctested.

dimpase commented 3 years ago
comment:50

Should we add the patch from ​https://github.com/Singular/Singular/issues/1089 here?

videlec commented 3 years ago
comment:51

Replying to @dimpase:

Should we add the patch from ​https://github.com/Singular/Singular/issues/1089 here?

According to your [comment:46] it does not solve the issue. So I don't see why we should only port this particular bug fix. Furthermore, we do not have any patch to singular in sage. Better keep it that way, no?

dimpase commented 3 years ago
comment:52

Replying to @videlec:

Replying to @dimpase:

Should we add the patch from ​https://github.com/Singular/Singular/issues/1089 here?

According to your [comment:46] it does not solve the issue. So I don't see why we should only port this particular bug fix. Furthermore, we do not have any patch to singular in sage. Better keep it that way, no?

isn't the bug uncovered in https://github.com/Singular/Singular/issues/1089 sitting in Singular code used by Sage, and may re-surface if not patched?

videlec commented 3 years ago
comment:53

Replying to @dimpase:

Replying to @videlec:

Replying to @dimpase:

Should we add the patch from ​https://github.com/Singular/Singular/issues/1089 here?

According to your [comment:46] it does not solve the issue. So I don't see why we should only port this particular bug fix. Furthermore, we do not have any patch to singular in sage. Better keep it that way, no?

isn't the bug uncovered in https://github.com/Singular/Singular/issues/1089 sitting in Singular code used by Sage, and may re-surface if not patched?

Maybe. I don't quite see the link with this ticket about subs and __call__. If you think it is worth adding this patch to singular, please open a ticket mentionning which issue it does solve on the sage side and create a branch.

dimpase commented 3 years ago
comment:54

here is another Singular leak demo, not fixed by the patch we discuss:

ring r;
map F=r,x+y+z3,y+z+x2z3,z+1+xyz;
poly f=(x+y+z+xz)^10;
matrix m=f;
matrix mm;
while (1) {mm=F(m);}

here I checked that the code path goes through fast_map_common_subexp(), so this is another leak, hopefully just what we are fighting on this ticket.

dimpase commented 3 years ago
comment:55

I've opened https://github.com/Singular/Singular/issues/1090

videlec commented 3 years ago

Description changed:

--- 
+++ 
@@ -31,6 +31,10 @@

 This was reported [on sage-devel](https://groups.google.com/forum/#!topic/sage-devel/iO1SzoW0kcw) and [ask.sagemath.org](https://ask.sagemath.org/question/29444/high-memory-usage-when-substituting-variables/).

-We fix `.subs` (the first example above) by modifying the appropriate singular call. For `__call__`, we identified a memory leak in singular, see [singular issue #1089](https://github.com/Singular/Singular/issues/1089). In the mean time, we use the non-satisfactory solution of going via naive substitution in Python. To do so, we moved the generic implementation on the base class `MPolynomial`.
+We fix `.subs` (the first example above) by modifying the appropriate singular call. We identified two possibly related memory leaks in singular
+- [singular issue #1089](https://github.com/Singular/Singular/issues/1089)
+- [singular issue #1090](https://github.com/Singular/Singular/issues/1090)
+
+In the mean time, we use the non-satisfactory solution of going via naive substitution in Python. To do so, we moved the generic implementation on the base class `MPolynomial`.

 See also #13447 also related to memory handling in singular.
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Changed commit from 394d173 to 58ba726

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

58ba72627261: more robust leak test
videlec commented 3 years ago
comment:58

Replying to @dimpase:

I've opened https://github.com/Singular/Singular/issues/1090

Apparently, partially fixed in b983a8a.

mwageringel commented 3 years ago
comment:59

This ask-sage question reports that there is also a performance problem with polynomial evaluation. Could this be related to the issue of this ticket? The example from the linked question is about 10 times slower than the naive Python substitution.

videlec commented 3 years ago
comment:60

Replying to @mwageringel:

This ask-sage question reports that there is also a performance problem with polynomial evaluation. Could this be related to the issue of this ticket? The example from the linked question is about 10 times slower than the naive Python substitution.

This ticket is not about performance issue. But as I mentioned in the ask question switching to naive Python evaluation as done in my branch actually speeds up the evaluation.

videlec commented 3 years ago
comment:61

ping?

dimpase commented 3 years ago
comment:62

testing...

dimpase commented 3 years ago
comment:63

lgtm

dimpase commented 3 years ago

Reviewer: Dima Pasechnik

videlec commented 3 years ago
comment:64

thx

dimpase commented 3 years ago
comment:65

one test broken on macOS:

File "src/sage/rings/polynomial/multi_polynomial_libsingular.pyx", line 153, in sage.rings.polynomial.multi_polynomial_libsingular
Failed example:
    for i in range(30):
        n = leak_subs(20)
        print("Leaked {} bytes".format(n))
        if n == 0:
            zeros += 1
            if zeros >= 6:
                print("done")
                break
        else:
            zeros = 0
Expected:
    Leaked ...
    ...
    Leaked 0 bytes
    done
Got:
    Leaked 184549376 bytes
    Leaked 12582912 bytes
    Leaked 4194304 bytes
    Leaked 0 bytes
    Leaked 0 bytes
    Leaked 8388608 bytes
    Leaked 0 bytes
    Leaked 37748736 bytes
    Leaked 0 bytes
    Leaked 4194304 bytes
    Leaked 0 bytes
    Leaked 0 bytes
    Leaked 4194304 bytes
    Leaked 0 bytes
    Leaked 0 bytes
    Leaked 4194304 bytes
    Leaked 0 bytes
    Leaked 0 bytes
    Leaked 37748736 bytes
    Leaked 4194304 bytes
    Leaked 0 bytes
    Leaked 0 bytes
    Leaked 4194304 bytes
    Leaked 0 bytes
    Leaked 0 bytes
    Leaked 37748736 bytes
    Leaked 4194304 bytes
    Leaked 0 bytes
    Leaked 0 bytes
    Leaked 4194304 bytes
**********************************************************************
1 item had failures:
   1 of  55 in sage.rings.polynomial.multi_polynomial_libsingular

(tested together with #30801, but I guess it doesn't matter.

videlec commented 3 years ago
comment:66

The sagemath-singular interface is worse than what I thought. Switching to a generic subs function also leak!

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

ddc9a6b27261: use generic subs
edf884727261: move tests in an independent file
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Changed commit from 58ba726 to edf8847

dimpase commented 3 years ago
comment:68

how about adding latest upstream fixes as patches?

dimpase commented 1 year ago

still leaks with Sage 10.1.beta6 (Singular 4.3.2) As get_memory_usage is gone, I'm running

sage: R = PolynomialRing(ZZ, 'x', 50)
sage: d = {str(g): g for g in R.gens()}
sage: p = sum(d.values())
sage: import gcsage: while (1):
....:     for _ in range(50):
....:         _ = p.subs(**d)
....:     _ = gc.collect()

and observing memory consumption growing in another ternimal running top

maxale commented 1 year ago

If someone wants to monitor memory leak directly in Sage, it can be done via psutil module. Here is a modified example:

import os
import gc
import psutil
import time

def _mem(pid=os.getpid()):
    gc.collect()
    process = psutil.Process(pid)
    return process.memory_full_info()

def test_leak():
    R = PolynomialRing(ZZ, 'x', 50)
    d = {str(g): g for g in R.gens()}
    p = sum(d.values())
    mytime = time.time()
    while True:
        _ = p.subs(**d)
        if time.time() - mytime > 10:   # every 10 seconds
            print(_mem())
            mytime = time.time()

Here test_leak() prints memory usage (after garbage collection) every 10 seconds, which goes like this:

pfullmem(rss=808443904, vms=2267947008, shared=79335424, text=2822144, lib=0, data=790425600, dirty=0, uss=788611072, pss=794329088, swap=0)

pfullmem(rss=1369354240, vms=2828382208, shared=79335424, text=2822144, lib=0, data=1350860800, dirty=0, uss=1349472256, pss=1355190272, swap=0)

pfullmem(rss=1940168704, vms=3398918144, shared=79335424, text=2822144, lib=0, data=1921396736, dirty=0, uss=1920376832, pss=1926094848, swap=0)