Opened 16 months ago
Closed 16 months ago
#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: |
Description (last modified by )
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
comment:2 Changed 16 months ago by
- Cc slelievre added
- Description modified (diff)
comment:3 Changed 16 months ago by
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
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
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
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
- Branch set to u/sbrandhorst/memory_leak_for_integrallattice
comment:8 Changed 16 months ago by
- Commit set to 3ee728ac827b31e62d40d4e89bff868a71b410cf
- Status changed from new to needs_review
New commits:
3ee728a | remove @cached_method to fix a memory leak for IntegralLattice
|
comment:9 Changed 16 months ago by
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
- Commit changed from 3ee728ac827b31e62d40d4e89bff868a71b410cf to bd6c256acc5a5e99f0fa2ad1111613b5c49bd0fd
Branch pushed to git repo; I updated commit sha1. New commits:
bd6c256 | indentation
|
comment:11 Changed 16 months ago by
- Reviewers set to Travis Scrimshaw
- Status changed from needs_review to positive_review
Thank you. LGTM.
comment:12 Changed 16 months ago by
- Priority changed from critical to blocker
comment:13 Changed 16 months ago by
- Branch changed from u/sbrandhorst/memory_leak_for_integrallattice to bd6c256acc5a5e99f0fa2ad1111613b5c49bd0fd
- Resolution set to fixed
- Status changed from positive_review to closed
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.