Opened 3 years ago

Last modified 5 weeks ago

#27261 needs_work defect

Memory leaks in libsingular polynomial evaluation and substitution

Reported by: SimonKing Owned by:
Priority: critical Milestone: sage-9.6
Component: memleak Keywords: libsingular polynomial memleak
Cc: nbruin, malb Merged in:
Authors: Vincent Delecroix, ​Dima Pasechnik Reviewers: Dima Pasechnik
Report Upstream: Reported upstream. No feedback yet. Work issues:
Branch: u/vdelecroix/27261 (Commits, GitHub, GitLab) Commit: edf88477b9986b2d476f08101f92450ac57bf58e
Dependencies: Stopgaps:

Status badges

Description (last modified by vdelecroix)

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.

Change History (70)

comment:1 Changed 3 years ago by SimonKing

For testing, I changed the code so that the id() of any newly created MPolynomial_libsingular instance is stored in some set, and is removed from the set as soon as it becomes garbage collected.

Result: In the above examples, the MPolynomial_libsingular instances are correctly garbage collected.

Conclusion: The memleak occurs in Sage's usage of libsingular. Apparently the underlying data of a polynomial are not correctly freed when it is deallocated.

comment:2 Changed 3 years ago by SimonKing

Or, another possibility: Some temporary libsingular data created during the computation of Q is not freed.

To detect this, I will try to make the example more fine-grained: Test if the leak occurs if the only arithmetic operation involved is _add_, _mul_ or __pow__, respectively. (X+Y)^120*Y^100 mixes these three operations.

comment:3 Changed 3 years ago by SimonKing

Interestingly, creating a copy of P from scratch does not leak:

sage: import gc
sage: R.<X,Y> = ZZ[]
sage: P = (X+Y)^120*Y^100
sage: mem0 = get_memory_usage()
sage: for i in range(500):
....:     Q = (X+Y)^120*Y^100
....:     del Q
....:     _ = gc.collect()
....:     mem1 = get_memory_usage()
....:     if mem1>mem0:
....:         print(i, mem1-mem0)
....:         mem0 = mem1
....:         
(0, 0.1484375)
sage: 

So, it seems that calling the polynomial (i.e., P(X,Y)) is what is leaking!

comment:4 Changed 3 years ago by SimonKing

Note that there previously was a leak in polynomial evaluation, as by comment in sage.libs.singular.polynomial.singular_polynomial_call - see #16958. So, perhaps it didn't get completely fixed?

comment:5 Changed 3 years ago by SimonKing

singular_polynomial_call copies the input data before doing the actual call. This, I think, is a waste of time. I tested that taking the copy is not causing the leak, but while we are at it, it could as well be improved, too.

comment:6 Changed 3 years ago by SimonKing

Also, singular_polynomial_call has an argument poly *(*get_element)(object) that is (at least in the Sage library) always assigned with the function MPolynomial_libsingular_get_element, which does nothing more than return the ._poly of an MPolynomial_libsingular.

So, basically the get_element argument is void. Should it be removed?

comment:7 Changed 3 years ago by SimonKing

  • Summary changed from Fix a memory leak in libsingular polynomial arithmetic to Fix a memory leak in libsingular polynomial evaluation

comment:8 Changed 3 years ago by embray

  • Milestone changed from sage-8.7 to sage-8.8

Ticket retargeted after milestone closed (if you don't believe this ticket is appropriate for the Sage 8.8 release please retarget manually)

comment:9 Changed 3 years ago by embray

  • Milestone sage-8.8 deleted

As the Sage-8.8 release milestone is pending, we should delete the sage-8.8 milestone for tickets that are not actively being worked on or that still require significant work to move forward. If you feel that this ticket should be included in the next Sage release at the soonest please set its milestone to the next release milestone (sage-8.9).

comment:10 Changed 9 months ago by vdelecroix

Stumble upon a similar problem using .subs()

sage: R.<X,Y> = ZZ[] 
....: import gc 
....: mem0 = get_memory_usage() 
....: for i in range(1000): 
....:     for _ in range(100): 
....:         _ = (X + Y).subs(X=X, Y=Y) 
....:     _ = gc.collect() 
....:     mem1 = get_memory_usage() 
....:     if mem1>mem0: 
....:         print(i, mem1-mem0) 
....:         mem0 = mem1                                                                                                                                                                      
78 0.1328125
100 0.1328125
121 0.12890625
142 0.12890625
164 0.1328125
172 2.0
185 0.12890625
206 0.12890625
228 0.1328125
...

See also https://ask.sagemath.org/question/29444/high-memory-usage-when-substituting-variables/

comment:11 Changed 9 months ago by vdelecroix

Since __call__ calls subs() I gues the latter is the culprit.

comment:12 Changed 9 months ago by vdelecroix

  • Branch set to u/vdelecroix/27261
  • Commit set to ee9a54a8467c6f6824a192ae7029af1c25feb4e6
  • Dependencies #13447 deleted
  • Milestone set to sage-9.4
  • Priority changed from major to critical
  • Status changed from new to needs_review

New commits:

ee9a54a27261: fix memory leak in polynomial substitution

comment:13 Changed 9 months ago by git

  • Commit changed from ee9a54a8467c6f6824a192ae7029af1c25feb4e6 to 00b85c82ee84dee8d4f5b7637e2c9b1080421610

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

00b85c827261: doctest subs leak

comment:14 Changed 9 months ago by git

  • Commit changed from 00b85c82ee84dee8d4f5b7637e2c9b1080421610 to bd4d41f86fb1a83761029666c5f90220355977f4

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

bd4d41f27261: doctest subs and __call__ leak

comment:15 Changed 9 months ago by vdelecroix

  • Authors set to Vincent Delecroix
  • Description modified (diff)

comment:16 follow-up: Changed 9 months ago by dimpase

Does gc.collect() trigger Singular's GC - does it even have such an option? It does its own memory management.

comment:17 in reply to: ↑ 16 ; follow-up: Changed 9 months ago by vdelecroix

Replying to dimpase:

Does gc.collect() trigger Singular's GC - does it even have such an option? It does its own memory management.

What does this have to do with the problem? The gc.collect() can be removed from the snippets. They are here to guarantee that there is no undeleted temporary Python objects lying around and get consistent tests. You can crash sage with

sage: R = PolynomialRing(ZZ, 'x', 50)
sage: d = {str(g): g for g in R.gens()}
sage: p = sum(d.values())
sage: while True:
....:     _ = p.subs(**d)
Last edited 9 months ago by vdelecroix (previous) (diff)

comment:18 Changed 9 months ago by dimpase

  • Status changed from needs_review to needs_work

The following 1-line patch fixes the leak from comment:10 and the 1st leak reported on the ticket.

  • src/sage/rings/polynomial/multi_polynomial_libsingular.pyx

    a b cdef class MPolynomial_libsingular(MPolynomial): 
    36203620                res_id = fast_map_common_subexp(from_id, _ring, to_id, _ring)
    36213621                _p = res_id.m[0]
    36223622
    3623                 from_id.m[0] = NULL
     3623                p_Delete(&from_id.m[0], _ring)
    36243624                res_id.m[0] = NULL
    3625 
    36263625                id_Delete(&from_id, _ring)
    36273626                id_Delete(&res_id, _ring)

it does not fix the 2nd leak on the ticket, but I guess it might be that one needs to clean more of these from_id parts...

comment:19 Changed 9 months ago by dimpase

  • Cc nbruin malb added

It'd be good to have more eyes on this, subs() code, preferably of people who wrote chunks of it.

comment:20 Changed 9 months ago by git

  • Commit changed from bd4d41f86fb1a83761029666c5f90220355977f4 to b4e22a43dfa4eb2bec4b7317a13b5d019153c6eb

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

19881aa27261: fix memory leak in polynomial substitution
b4e22a427261: doctest subs and __call__ leak

comment:21 Changed 9 months ago by vdelecroix

  • Status changed from needs_work to needs_review

thx for your input!

comment:22 in reply to: ↑ 17 ; follow-up: Changed 9 months ago by dimpase

Replying to vdelecroix:

Replying to dimpase:

Does gc.collect() trigger Singular's GC - does it even have such an option? It does its own memory management.

What does this have to do with the problem? The gc.collect() can be removed from the snippets. They are here to guarantee that there is no undeleted temporary Python objects lying around and get consistent tests. You can crash sage with

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

with the patch I just proposed this can be run seemingly forever, without showing any memory increase after few minutes of CPU time.

comment:23 Changed 9 months ago by git

  • Commit changed from b4e22a43dfa4eb2bec4b7317a13b5d019153c6eb to fbc22ce52ee92231520303b8d90a58fb1464833f

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

fbc22ce27261: fix leak tests

comment:24 in reply to: ↑ 22 Changed 9 months ago by vdelecroix

  • Authors changed from Vincent Delecroix to Vincent Delecroix, ​Dima Pasechnik

Replying to dimpase:

Replying to vdelecroix:

Replying to dimpase:

Does gc.collect() trigger Singular's GC - does it even have such an option? It does its own memory management.

What does this have to do with the problem? The gc.collect() can be removed from the snippets. They are here to guarantee that there is no undeleted temporary Python objects lying around and get consistent tests. You can crash sage with

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

with the patch I just proposed this can be run seemingly forever, without showing any memory increase after few minutes of CPU time.

Very good. This is implemented in commit 19881aa.

comment:25 Changed 9 months ago by dimpase

seems to be a similar bug, from_id, to_id stuff in singular_polynomial_call. Here is how to trigger it there, (needs --long)

  • src/sage/libs/singular/polynomial.pyx

    a b cdef int singular_polynomial_call(poly **ret, poly *p, ring *r, list args, poly 
    167167        sage: import gc
    168168        sage: F.<a> = GF(7^2)
    169169        sage: R.<x,y> = F[]
    170         sage: p = x+2*y
     170        sage: p = (x+2*y)^3
    171171        sage: def leak(N):
    172172        ....:     before = resource.getrusage(resource.RUSAGE_SELF).ru_maxrss
    173173        ....:     gc.collect()
    174174        ....:     for i in range(N):
    175         ....:         _ = p(a, a)
     175        ....:         _ = p(a+x, a)
    176176        ....:     after = resource.getrusage(resource.RUSAGE_SELF).ru_maxrss
    177177        ....:     return (after - before) * 1024   # ru_maxrss is in kilobytes

gives

File "src/sage/libs/singular/polynomial.pyx", line 187, in sage.libs.singular.polynomial.singular_polynomial_call
Failed example:
    for i in range(30):  # long time
        n = leak(10000)
        print("Leaked {} bytes".format(n))
        if n == 0:
            zeros += 1
            if zeros >= 6:
                break
        else:
            zeros = 0
Expected:
    Leaked...
    Leaked 0 bytes
    Leaked 0 bytes
    Leaked 0 bytes
    Leaked 0 bytes
    Leaked 0 bytes
Got:
    Leaked 6180864 bytes
    Leaked 3956736 bytes
    Leaked 3923968 bytes
    Leaked 3923968 bytes
...

comment:26 Changed 9 months ago by git

  • Commit changed from fbc22ce52ee92231520303b8d90a58fb1464833f to 3ffc6dd98efc7375429839b36c338a59580bd6c2

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

3ffc6dd27261: move call test in sage/libs/singular

comment:27 Changed 9 months ago by vdelecroix

True. I moved my test over ZZ in the same file.

comment:28 Changed 9 months ago by dimpase

unfortunately I don't know how to fix the leak in singular_polynomial_call(). Trying to do something similar leads tp segfaults.

comment:29 Changed 9 months ago by vdelecroix

  • Description modified (diff)
  • Summary changed from Fix a memory leak in libsingular polynomial evaluation to Memory leaks in libsingular polynomial evaluation and substitution

Thanks for trying. I update the description. Should we agree on the current status? Should we make it for sage-9.3?

comment:30 Changed 9 months ago by git

  • Commit changed from 3ffc6dd98efc7375429839b36c338a59580bd6c2 to 1bf94f436804e6cbf34772c771353217cac9e6c8

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

1bf94f427261: coerce argument to parent if possible

comment:31 Changed 9 months ago by gh-mwageringel

I believe this indicates that the problem is upstream, in fast_map_common_subexp. I have replaced the function by one that does nothing but return a copy of the original polynomial.

  • src/sage/libs/singular/polynomial.pyx

    a b cdef int singular_polynomial_call(poly **ret, poly *p, ring *r, list args, poly 
    173173        ....:     for i in range(N):
    174174        ....:         _ = p(x, y)
    175175        ....:         _ = p(x + y, y)
    176         ....:         _ = p(1, -1)
    177         ....:         _ = p(0, 0)
    178176        ....:     gc.collect()
    179177        ....:     after = resource.getrusage(resource.RUSAGE_SELF).ru_maxrss
    180178        ....:     return (after - before) * 1024   # ru_maxrss is in kilobytes
    cdef int singular_polynomial_call(poly **ret, poly *p, ring *r, list args, poly 
    185183        ....:     gc.collect()
    186184        ....:     before = resource.getrusage(resource.RUSAGE_SELF).ru_maxrss
    187185        ....:     for i in range(N):
    188         ....:         _ = p(a, a)
    189186        ....:         _ = p(x, y)
    190187        ....:         _ = p(x + y, y)
    191188        ....:         _ = p(a + x, a)
    cdef int singular_polynomial_call(poly **ret, poly *p, ring *r, list args, poly 
    228225    from_id.m[0] = p
    229226
    230227    rChangeCurrRing(r)
    231     cdef ideal *res_id = fast_map_common_subexp(from_id, r, to_id, r)
     228    cdef ideal *res_id = dummy_map_common_subexp(from_id, r, to_id)
    232229    ret[0] = res_id.m[0]
    233230
    234231    # Unsure why we have to normalize here. See #16958
    cdef int singular_polynomial_call(poly **ret, poly *p, ring *r, list args, poly 
    243240
    244241    return 0
    245242
     243# can fail if the result is expected to be constant
     244cdef ideal *dummy_map_common_subexp(ideal *from_id, ring *r, ideal *to_id):
     245    cdef ideal *res_id = idInit(1, 1)
     246    res_id.m[0] = p_Copy(from_id.m[0], r)
     247    return res_id
     248
    246249cdef int singular_polynomial_cmp(poly *p, poly *q, ring *r):
    247250    """
    248251    Compare two Singular elements ``p`` and ``q`` in ``r``.
Got:
    Leaked 0 and 0 bytes
    Leaked 0 and 0 bytes
    Leaked 0 and 0 bytes
    Leaked 0 and 0 bytes
    Leaked 0 and 0 bytes
    Leaked 0 and 0 bytes

comment:32 Changed 9 months ago by dimpase

How does one convince the upstream? Would writing a C++ program using libsingular suffice? Or a Singular example would be required? It could be that Singular does not use this function the way it's used here. In fact, in Singular it's only called at one place, once, in in maMapIdeal() And the latter is only called once:

./Singular/ipshell.cc:  v->data=maMapIdeal(IDIDEAL(w), src_ring, (ideal)theMap, currRing,nMap);

in Singular's shell, so it's probably not too hard to figure out the Singular command calling it.

comment:33 Changed 9 months ago by git

  • Commit changed from 1bf94f436804e6cbf34772c771353217cac9e6c8 to f90202c7ef1d9926cc6a9bb5df2811e169fbbb4f

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

f90202c27261: be more careful about parent of result

comment:34 follow-up: Changed 9 months ago by dimpase

Singular's subst() leaks memory very fast, I don't know why we expect anything better from libsingular. E.g.

ring r=0,(x,y),dp;
poly p=(x+y)^10;
while (1) {
subst(p,x,x+y);
}

takes 30 sec to reach many gigabytes.

comment:35 in reply to: ↑ 34 Changed 9 months ago by vdelecroix

  • Description modified (diff)
  • Report Upstream changed from N/A to Reported upstream. No feedback yet.

I opened an issue on the singular side https://github.com/Singular/Singular/issues/1089.

comment:36 follow-up: Changed 9 months ago by vdelecroix

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

comment:37 in reply to: ↑ 36 ; follow-up: Changed 9 months ago by vdelecroix

Replying to vdelecroix:

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

comment:38 in reply to: ↑ 37 Changed 9 months ago by vdelecroix

Replying to vdelecroix:

Replying to vdelecroix:

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

comment:39 Changed 8 months ago by git

  • Commit changed from f90202c7ef1d9926cc6a9bb5df2811e169fbbb4f to 796e85480c55e3cccec5acbfb88ebf92e111ae4b

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

comment:40 Changed 8 months ago by vdelecroix

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.

comment:41 Changed 8 months ago by git

  • Commit changed from 796e85480c55e3cccec5acbfb88ebf92e111ae4b to 394d1734d7b939737bd8549558cc41cde91e59b4

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

ed83ed327261: more robust doctest in polynomial.pyx
394d17327261: fix error message in __call

comment:42 Changed 8 months ago by vdelecroix

  • Description modified (diff)

comment:44 in reply to: ↑ 43 Changed 8 months ago by vdelecroix

comment:45 Changed 8 months ago by vdelecroix

  • Report Upstream changed from Reported upstream. No feedback yet. to Fixed upstream, but not in a stable release.

comment:46 follow-up: Changed 8 months ago by dimpase

  • Report Upstream changed from Fixed upstream, but not in a stable release. to Reported upstream. No feedback yet.

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

comment:47 in reply to: ↑ 46 Changed 8 months ago by vdelecroix

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.

comment:48 follow-up: Changed 8 months ago by dimpase

are the leaks in the ticket text now fixed?

comment:49 in reply to: ↑ 48 Changed 8 months ago by vdelecroix

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.

comment:50 follow-up: Changed 8 months ago by dimpase

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

comment:51 in reply to: ↑ 50 ; follow-up: Changed 8 months ago by vdelecroix

Replying to dimpase:

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

According to your 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?

comment:52 in reply to: ↑ 51 ; follow-up: Changed 8 months ago by dimpase

Replying to vdelecroix:

Replying to dimpase:

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

According to your 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?

comment:53 in reply to: ↑ 52 Changed 8 months ago by vdelecroix

Replying to dimpase:

Replying to vdelecroix:

Replying to dimpase:

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

According to your 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.

comment:54 follow-up: Changed 8 months ago by dimpase

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.

comment:56 in reply to: ↑ 54 Changed 8 months ago by vdelecroix

  • Description modified (diff)

comment:57 Changed 8 months ago by git

  • Commit changed from 394d1734d7b939737bd8549558cc41cde91e59b4 to 58ba726c305e9c8136d15173e5cab2e9b55cc9fd

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

58ba72627261: more robust leak test

comment:58 in reply to: ↑ 55 Changed 8 months ago by vdelecroix

Replying to dimpase:

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

Apparently, partially fixed in b983a8a.

comment:59 follow-up: Changed 8 months ago by gh-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.

Last edited 8 months ago by gh-mwageringel (previous) (diff)

comment:60 in reply to: ↑ 59 Changed 8 months ago by vdelecroix

Replying to gh-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.

comment:61 Changed 8 months ago by vdelecroix

ping?

comment:62 Changed 8 months ago by dimpase

testing...

comment:63 Changed 8 months ago by dimpase

  • Reviewers set to Dima Pasechnik
  • Status changed from needs_review to positive_review

lgtm

comment:64 Changed 8 months ago by vdelecroix

thx

comment:65 Changed 8 months ago by dimpase

  • Status changed from positive_review to needs_work

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.

comment:66 Changed 8 months ago by vdelecroix

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

comment:67 Changed 8 months ago by git

  • Commit changed from 58ba726c305e9c8136d15173e5cab2e9b55cc9fd to edf88477b9986b2d476f08101f92450ac57bf58e

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

ddc9a6b27261: use generic subs
edf884727261: move tests in an independent file

comment:68 Changed 8 months ago by dimpase

how about adding latest upstream fixes as patches?

comment:69 Changed 5 months ago by mkoeppe

  • Milestone changed from sage-9.4 to sage-9.5

comment:70 Changed 5 weeks ago by mkoeppe

  • Milestone changed from sage-9.5 to sage-9.6
Note: See TracTickets for help on using tickets.