Opened 3 years ago

Closed 3 months ago

#27083 closed defect (invalid)

memory leak, possibly in crystals

Reported by: mantepse Owned by:
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: memleak Keywords: memory leak
Cc: tscrim, slelievre Merged in:
Authors: Reviewers: Samuel Lelièvre
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by tscrim)

As noticed in #27057:

sage: import gc
sage: C = crystals.Letters("A5")
sage: for d in range(10,1,-1):
....:     T = crystals.TensorProduct(*([C]*d))
....:     hi = T.highest_weight_vectors()
....:     _ = gc.collect()
....:     _ = gc.collect()
....:     print get_memory_usage(), len(hi)
....:     
5321.24609375 9096
5322.49609375 2556
5322.74609375 756
5322.99609375 231
5322.99609375 76
5322.99609375 26
5322.99609375 10
5322.99609375 4
5322.99609375 2
sage: C = crystals.Letters(['A', 1])
sage: C._dummy = True
sage: del C
sage: import gc
sage: _ = gc.collect()
sage: C = crystals.Letters(['A', 1])
sage: C._dummy
True
sage: C = ShiftedPrimedTableaux([2,1], max_entry=2)
sage: C._dummy = True
sage: del C
sage: _ = gc.collect()
sage: C = ShiftedPrimedTableaux([2,1], max_entry=2)
sage: C._dummy
True

Change History (14)

comment:1 Changed 3 years ago by mantepse

Actually, I think this is to be expected with UniqueRepresentation, no?

sage: C = SetPartitions()
sage: C._dummy = True
sage: del C
sage: C = SetPartitions()
sage: C._dummy
True

comment:2 follow-up: Changed 3 years ago by jdemeyer

How is this a memory leak? Why is this a bug?

comment:3 in reply to: ↑ 2 Changed 3 years ago by nbruin

Replying to jdemeyer:

How is this a memory leak? Why is this a bug?

It is a little suspicious because the UniqueRepresentation object C is deleted, so one would assume C should now be unreachable. Then gc.collect() is run, which should clean unreachable objects. So you'd expect after that to get a clean copy of C when it is recreated. But as you can see, it is not: it still has this _dummy attribute.

Testing creating and deleting these objects in a loop and then checking if any pile up on the heap using gc.get_objects doesn't give worrisome results, however. And when WeakRefs are involved it's actually possible to prevent gc from collecting everything in one swoop, so I agree there doesn't seem to be a memory leak.

But I think the report had some merit to it in that it pointed out suspicious behaviour that warrants a little further investigation (which found nothing).

comment:4 follow-up: Changed 3 years ago by tscrim

  • Description modified (diff)

Martin, it is *not* because of UniqueRepresentation. Although I am a little surprised that running gc.collect() a few times after deleting it is not leading to a new object being created:

sage: P = GF(3037000453)['x','y']
sage: P.dummy = True
sage: P.dummy
True
sage: del P
sage: gc.collect() # I ran this 5 times
sage: P = GF(3037000453)['x','y']
sage: P.dummy
...
AttributeError

Although in this example, it is not a UniqueRepresentation object, but I think it illustrates what should happen.

I am also wondering if this is related to #18426.

comment:5 follow-up: Changed 3 years ago by tscrim

I do not seem to be able to actually free the memory in the loop mentioned.

Nils, can you remind me the code of how to check with gc.get_objects()? I can never seem to remember it.

comment:6 Changed 3 years ago by tscrim

  • Description modified (diff)

I flopped the direction of the loop to illustrate the problem more. Maybe the get_memory_usage is not the right test...

comment:7 in reply to: ↑ 4 Changed 3 years ago by mantepse

Replying to tscrim:

Martin, it is *not* because of UniqueRepresentation.

I only thought so because you can reproduce it also with other UniqueRepresentation instances. For example:

sage: C = SetPartitions()
sage: C.dummy = True
sage: del C
sage: gc.collect()
sage: C = SetPartitions()
sage: C.dummy
True

comment:8 in reply to: ↑ 5 ; follow-up: Changed 3 years ago by nbruin

Replying to tscrim:

I do not seem to be able to actually free the memory in the loop mentioned.

Nils, can you remind me the code of how to check with gc.get_objects()? I can never seem to remember it.

There's a pretty clean example on the ticket #18426 you referenced above and which you reported. That's where I copied the code from when I tried. I actually made a mistake. There does seem to be some leakage, but it seems capped, oddly enough; like there's an LRU cache in place or so.

import gc
from collections import Counter
gc.collect()
pre={id(a) for a in gc.get_objects()}
for n in [2..600]:
  r = ShiftedPrimedTableaux([n,1], max_entry=2)
  r._dummy= True
del r
gc.collect()
gc.collect()
gc.collect()

T=Counter(str(type(a)) for a in gc.get_objects() if id(a) not in pre)
[t for t in T.iteritems() if 'Primed' in t[0]]

For me on 8.6, this finds 128 objects. And that number remains constant if I increase the bound 600 above.

comment:9 in reply to: ↑ 8 Changed 3 years ago by tscrim

  • Component changed from combinatorics to memleak
  • Milestone changed from sage-8.7 to sage-duplicate/invalid/wontfix
  • Status changed from new to needs_review

Replying to nbruin:

Replying to tscrim:

I do not seem to be able to actually free the memory in the loop mentioned.

Nils, can you remind me the code of how to check with gc.get_objects()? I can never seem to remember it.

There's a pretty clean example on the ticket #18426 you referenced above and which you reported.

*facepalm*

That's where I copied the code from when I tried. I actually made a mistake. There does seem to be some leakage, but it seems capped, oddly enough; like there's an LRU cache in place or so.

Running this variant

import gc
from collections import Counter
gc.collect()
pre={id(a) for a in gc.get_objects()}
for n in range(1000,2,-1):
    C = crystals.Letters(['A',n])
    T = tensor([C]*n)
    del C
    del T
gc.collect()
gc.collect()
gc.collect()

T=Counter(str(type(a)) for a in gc.get_objects() if id(a) not in pre)
[t for t in T.iteritems() if 'letters' in t[0] or 'tensor_product' in t[0]]

results in

[("<type 'sage.combinat.crystals.letters.Crystal_of_letters_type_A_element'>",
  624),
 ("<class 'sage.combinat.crystals.tensor_product.FullTensorProductOfRegularCrystals_with_category'>",
  32),
 ("<class 'sage.combinat.crystals.letters.ClassicalCrystalOfLetters_with_category'>",
  32)]
sage: sum(crystals.Letters(['A',n]).cardinality() for n in range(3,35))
624

So I agree, there seems to be some sort of LRU cache going on, but no real memory leak.

Actually, I have vague recollection of some sort of discussion about doing an LRU cache on sage-devel at some point, but I don't remember the outcome or details.

comment:10 Changed 3 years ago by mantepse

Replacing ShiftedPrimedTableaux([n,1], max_entry=2) with SetPartitions(n) I get <class 'sage.combinat.set_partition.SetPartitions_set_with_category'>": 128.

Perhaps the thread https://groups.google.com/forum/#!searchin/sage-devel/lru/sage-devel/q5uy_lI11jg/kRWKxvCImwEJ from long ago is relevant?

comment:11 Changed 3 years ago by nbruin

Oh boy. It would seem that #24954 is responsible for this:

unique_representation.py:1012

indeed it mentions a "128". So yes, there is no memory leak in the code, but after #24954 any test will indeed perceive a leak of something like 128 elements. In fact, with crystal.Letters there is apparently another implied construction, so we end up with only "64" elements.

This was exactly why I wasn't happy with #24954. It makes finding memory error that much harder. On the plus side: as this example shows, it does drive home the global nature of UniqueRepresentation elements a little more, and the fact that you shouldn't add or modify attributes on them.

Last edited 3 months ago by slelievre (previous) (diff)

comment:12 Changed 3 years ago by jdemeyer

See #24954 indeed.

comment:13 Changed 3 months ago by slelievre

  • Cc slelievre added
  • Reviewers set to Samuel Lelièvre
  • Status changed from needs_review to positive_review

Let us close this if nobody objects.

comment:14 Changed 3 months ago by mkoeppe

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