Opened 4 years ago

Closed 4 years ago

#20268 closed defect (fixed)

memory leak in Polynomial.__call__

Reported by: vdelecroix Owned by:
Priority: blocker Milestone: sage-duplicate/invalid/wontfix
Component: performance Keywords:
Cc: nbruin, jdemeyer, vbraun Merged in:
Authors: Reviewers: Vincent Delecroix
Report Upstream: N/A Work issues:
Branch: u/vdelecroix/20268 (Commits) Commit: 4f15c250d5efa6e6d4a6e1cba1830cdf72519c34
Dependencies: Stopgaps:

Description (last modified by vdelecroix)

The following cause a memory leak

sage: R = PolynomialRing(ZZ, 'x', implementation='NTL')
sage: x = R.gen()
sage: p = x**2 - 1
sage: while True: a = p(1)

See the following reports

We backport the fix provided in this commit.

Change History (13)

comment:1 Changed 4 years ago by vdelecroix

  • Authors set to Vincent Delecroix
  • Branch set to u/vdelecroix/20268
  • Commit set to 2a2e0a112a7bfb1bd2d91d5f344d4ba10e4db58d
  • Description modified (diff)
  • Status changed from new to needs_review

New commits:

2a2e0a1Trac 20268: upstream patch for memory leak

comment:2 Changed 4 years ago by jdemeyer

I propose to deal with this in #20192 instead.

In any case, if you want the fix to be applied, you need to increase the Cython version number.

comment:3 Changed 4 years ago by jdemeyer

  • Status changed from needs_review to needs_work

comment:4 Changed 4 years ago by git

  • Commit changed from 2a2e0a112a7bfb1bd2d91d5f344d4ba10e4db58d to 4f15c250d5efa6e6d4a6e1cba1830cdf72519c34

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

4f15c25Trac 20268: bump cython version

comment:5 Changed 4 years ago by vdelecroix

  • Status changed from needs_work to needs_review

Since the stable cython release is about to be delivered, I guess it would be better to use #20192...

comment:6 Changed 4 years ago by jdemeyer

Robert Bradshaw says that Cython 0.24 will be delivered "Likely in the next week"

comment:7 Changed 4 years ago by vdelecroix

  • Dependencies set to #20192
  • Status changed from needs_review to needs_work

I would like to add a doctest for that. Do you think that the following is strong enough

sage: p = polygen(ZZ)
sage: import resource 
sage: mem0 = resource.getrusage(resource.RUSAGE_SELF).ru_maxrss 
sage: for _ in range(10000):
....:     t = Polynomial.__call__(p, 1)
sage: assert resource.getrusage(resource.RUSAGE_SELF).ru_maxrss < mem0 + 1000

If not, do you have a better idea?

comment:8 follow-up: Changed 4 years ago by nbruin

Perhaps staying closer to python's own memory management routines and pick an example that produces stuff that is tracked by gc:

sage: import gc
sage: p=polygen(ZZ)
sage: gc.collect()
0
sage: N=len(gc.get_objects())
sage: for _ in range(10000): t = Polynomial.__call__(p,p)
sage: assert len(gc.get_objects())-N < 1000

comment:9 in reply to: ↑ 8 ; follow-up: Changed 4 years ago by vdelecroix

Replying to nbruin:

Perhaps staying closer to python's own memory management routines and pick an example that produces stuff that is tracked by gc:

sage: import gc
sage: p=polygen(ZZ)
sage: gc.collect()
0
sage: N=len(gc.get_objects())
sage: for _ in range(10000): t = Polynomial.__call__(p,p)
sage: assert len(gc.get_objects())-N < 1000

It will not work. The leak was not seen at Python level.

comment:10 in reply to: ↑ 9 Changed 4 years ago by nbruin

Replying to vdelecroix:

It will not work. The leak was not seen at Python level.

Did you try the example? The leak causes dangling objects on the python heap. However, gc.get_objects() will only find tracked objects, and tuples with too simple ingredients aren't tracked. However, when you put sufficiently complicated objects in there (such as p) then it is tracked. So the test does work. So should the rusage of course, modulo unpredictable memory allocation strategies, or unusual circumstances that cause the rss to remain small in the presence of large memory usage.

comment:11 Changed 4 years ago by jdemeyer

  • Dependencies #20192 deleted

comment:12 Changed 4 years ago by jdemeyer

  • Authors Vincent Delecroix deleted
  • Milestone changed from sage-7.2 to sage-duplicate/invalid/wontfix
  • Reviewers set to Vincent Delecroix
  • Status changed from needs_work to positive_review

I guess this is superseded by the Cython upgrade.

comment:13 Changed 4 years ago by vbraun

  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.