#31625 closed defect (fixed)

Memory leak for IntegralLattice

Reported by: sbrandhorst Owned by:
Priority: blocker Milestone: sage-9.3
Component: memleak Keywords:
Cc: tscrim, nbruin, jdemeyer, slelievre 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 slelievre)

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 16 months ago by sbrandhorst

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 16 months ago by slelievre

  • Cc slelievre added
  • Description modified (diff)

comment:3 Changed 16 months ago by tscrim

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 16 months ago by sbrandhorst

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 16 months ago by sbrandhorst

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 16 months ago by tscrim

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

comment:7 Changed 16 months ago by sbrandhorst

  • Branch set to u/sbrandhorst/memory_leak_for_integrallattice

comment:8 Changed 16 months ago by sbrandhorst

  • Authors set to Simon Brandhorst
  • Commit set to 3ee728ac827b31e62d40d4e89bff868a71b410cf
  • Status changed from new to needs_review

New commits:

3ee728aremove @cached_method to fix a memory leak for IntegralLattice

comment:9 Changed 16 months ago by tscrim

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 16 months ago by git

  • Commit changed from 3ee728ac827b31e62d40d4e89bff868a71b410cf to bd6c256acc5a5e99f0fa2ad1111613b5c49bd0fd

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

bd6c256indentation

comment:11 Changed 16 months ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

Thank you. LGTM.

comment:12 Changed 16 months ago by mkoeppe

  • Priority changed from critical to blocker

comment:13 Changed 16 months ago by vbraun

  • Branch changed from u/sbrandhorst/memory_leak_for_integrallattice to bd6c256acc5a5e99f0fa2ad1111613b5c49bd0fd
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.