#31625 closed defect (fixed)

Memory leak for IntegralLattice

Reported by: Simon Brandhorst Owned by:
Priority: blocker Milestone: sage-9.3
Component: memleak Keywords:
Cc: Travis Scrimshaw, Nils Bruin, Jeroen Demeyer, Samuel Lelièvre Merged in:
Authors: Simon Brandhorst Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: bd6c256 (Commits, GitHub, GitLab) Commit: bd6c256acc5a5e99f0fa2ad1111613b5c49bd0fd
Dependencies: Stopgaps:

Status badges

Description (last modified by Samuel Lelièvre)

Computing the discriminant group of an integral lattice causes both to persist in memory.

sage: L = IntegralLattice("A2") 
sage: for k in range(1,500): 
....:     G = L.twist(k) 
sage: gc.collect() 
sage: len([a for a in gc.get_objects() if type(a)==type(L)])                                                                                                                                       
1058
3
sage: L = IntegralLattice("A2") 
sage: for k in range(1,500): 
....:     G = L.twist(k) 
....:     D = G.discriminant_group() 
sage: gc.collect() 
sage: len([a for a in gc.get_objects() if type(a)==type(L)])                                                                                                                                       
1284
501

Creating thousands of lattices and computing their discriminant groups is actually a common use case. Serious computations are impossible with this kind of memory leak.

Change History (13)

comment:1 Changed 20 months ago by Simon Brandhorst

L.discriminant_group() is cached. Removing the cache is a possible fix to the leak. However it seems very natural for a lattice to remember its discriminant group. And is basically the only reasonable way to create such things. On the other hand the discriminant group knows about its lattice covering lattice (it has to).

Another option to fix the leak is to remove CachedRepresentation from the discriminant group.

Not sure how to proceed here. And there are other instances of this kind of memory leak.

comment:2 Changed 20 months ago by Samuel Lelièvre

Cc: Samuel Lelièvre added
Description: modified (diff)

comment:3 Changed 20 months ago by Travis Scrimshaw

I would probably remove the @cached_method from discriminant_group(). I don't see immediately why there would be a need to cache that. It doesn't seem like something you call frequently from different parts of the code. You could just tweak maximal_overlattice() in order to avoid repeated calls (which I would argue is better coding style anyways). (Similarly, you don't cache the orthogonal_group().)

comment:4 Changed 20 months ago by Simon Brandhorst

discriminant_group() is something that is called quite frequently by the user. Creating it is not too expensive so okay.

But computing generators for the orthogonal_group() is very expensive. So I think here unique representation does not help a lot. Storing it on its domain seems better.

comment:5 Changed 20 months ago by Simon Brandhorst

I guess I can let the unique representation machinery do the caching and only store the generators on the orthogonal group. So that is fine. Final question how best to doctest that a memory leak is fixed?

comment:6 Changed 20 months ago by Travis Scrimshaw

You could use the test in the ticket description, but it doesn't necessarily need a doctest IMO.

comment:7 Changed 20 months ago by Simon Brandhorst

Branch: u/sbrandhorst/memory_leak_for_integrallattice

comment:8 Changed 20 months ago by Simon Brandhorst

Authors: Simon Brandhorst
Commit: 3ee728ac827b31e62d40d4e89bff868a71b410cf
Status: newneeds_review

New commits:

3ee728aremove @cached_method to fix a memory leak for IntegralLattice

comment:9 Changed 20 months ago by Travis Scrimshaw

Your """ is not aligned properly (see the diff) and your added doctests need to be indented.

(I am slightly squeamish about adding such a doctest for the memory leak, but not enough to not include it.)

comment:10 Changed 20 months ago by git

Commit: 3ee728ac827b31e62d40d4e89bff868a71b410cfbd6c256acc5a5e99f0fa2ad1111613b5c49bd0fd

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

bd6c256indentation

comment:11 Changed 20 months ago by Travis Scrimshaw

Reviewers: Travis Scrimshaw
Status: needs_reviewpositive_review

Thank you. LGTM.

comment:12 Changed 20 months ago by Matthias Köppe

Priority: criticalblocker

comment:13 Changed 20 months ago by Volker Braun

Branch: u/sbrandhorst/memory_leak_for_integrallatticebd6c256acc5a5e99f0fa2ad1111613b5c49bd0fd
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.