avoid creating unneeded homsets when coercing

[sagetrac-mabshoff]

Burcin reported at #4639:

sage: F = GF(13)
sage: get_memory_usage()
708.02734375
sage: for _ in xrange(10000):
....:     t = F.coerce(F(234234))
....:     
sage: get_memory_usage()
728.15234375
sage: for _ in xrange(100000):
    t = F.coerce(F(234234))
....:     
sage: get_memory_usage()
932.3125
sage: for _ in xrange(100000):
    t = F.coerce(F(234234))
....:     
sage: get_memory_usage()
1136.35546875

Since the patch by RobertWB at that ticket fixes the issue Burcin reported, but not the original one I am moving it over to this ticket.

Cheers,

Michael

Apply

attachment: trac4740-cache_coerce_from_self.patch​

Depends on #14519

CC: @burcin @robertwb @simon-king-jena

Component: memleak

Author: Simon King

Reviewer: Mike Hansen

Merged: sage-5.12.beta3

Issue created by migration from https://trac.sagemath.org/ticket/4740

[sagetrac-mabshoff]

Attachment: trac_4740_coerce-leak.patch.gz

Patch by RobertWB, originally from #4639

[sagetrac-mabshoff]

comment:1

Patch by RobertWB, review by William.

Cheers,

Michael

[sagetrac-mabshoff]

comment:2

This patch causes a failure with the pickle jar:

**********************************************************************
File "/scratch/mabshoff/release-cycle/sage-3.2.2.alpha1/devel/sage/sage/structure/sage_object.pyx", line 371, in __main__.example_16
Failed example:
    sage.structure.sage_object.unpickle_all(std)###line 682:_sage_    >>> sage.structure.sage_object.unpickle_all(std)
Expected:
    doctest:...: DeprecationWarning: Your data is stored in an old format. Please use the save() function to store your data in a more recent format.
    Successfully unpickled ... objects.
    Failed to unpickle 0 objects.
Got:
    doctest:1172: DeprecationWarning: Your data is stored in an old format. Please use the save() function to store your data in a more recent format.
    ** failed:  _class__sage_rings_finite_field_morphism_FiniteFieldHomset__.sobj
    Failed:
    _class__sage_rings_finite_field_morphism_FiniteFieldHomset__.sobj
    Successfully unpickled 453 objects.
    Failed to unpickle 1 objects.
**********************************************************************
1 items had failures:
   1 of   7 in __main__.example_16

Cheers,

Michael

[sagetrac-mabshoff]

comment:3

Once the pickling issue is fixed this should go in.

Cheers,

Michael

[sagetrac-mabshoff]

comment:4

Robert,

the leak seems to be gone in 3.2.2:

----------------------------------------------------------------------
| Sage Version 3.2.2, Release Date: 2008-12-18                       |
| Type notebook() for the GUI, and license() for information.        |
----------------------------------------------------------------------
sage: F = GF(13)
sage: get_memory_usage()
689.02734375
sage: for _ in xrange(10000):
....:     t = F.coerce(F(234234))
....:     
sage: get_memory_usage()
689.02734375
sage: for _ in xrange(100000):
....:     t = F.coerce(F(234234))
....:     
sage: get_memory_usage()
689.02734375
sage: for _ in xrange(100000):
....:     t = F.coerce(F(234234))
....:     
sage: get_memory_usage()
689.02734375
sage: 

Should we close this ticket as won't fix or is the patch her still relevant? In that case we should update the ticket to reflect the actual issue being fixed here.

Cheers,

Michael

[robertwb]

comment:5

The patch is still relevant here. What is going on is every time coerce is called, a new homset is created. Creating a new homset doesn't leak anymore, but it's still sub-optimal.

I don't know that I'll have time to look into this anytime soon, but it's not a blocker anymore as the leak is solved elsewhere.

[sagetrac-mabshoff]

comment:6

ok,

I renamed the ticket then.

Cheers,

Michael

[simon-king-jena]

comment:7

Hi Robert,

accidentally I found this rather old ticket.

Replying to @robertwb:

The patch is still relevant here. What is going on is every time coerce is called, a new homset is created. Creating a new homset doesn't leak anymore, but it's still sub-optimal.

Is this still the case? In what situation is the homset created? Is it not cached? In what files does it occur?

Cheers,
Simon

[koffie]

comment:8

I also found this ticket. It could be very well that this is still causing memory leaks. Look for example at the reference chain that is keeping ModularSymbols alive at #10548

[simon-king-jena]

comment:20

I didn't notice that self.coerce_map_from(S) also has a special case when S is self. However, the result is again not cached.

        if S is self:
            from sage.categories.homset import Hom
            return Hom(self, self).identity()

[simon-king-jena]

comment:21

Replying to @simon-king-jena:

2. let F.coerce(x) avoid creating a morphism, if the parent of x is F.

Hm. On second thought, do we really want that F.coerce(x) is x? I guess it is better to preserve the current behaviour and return a copy of x.

[simon-king-jena]

Changed author from Mike Hansen to Simon King

[simon-king-jena]

comment:22

What do people think about the patch that I just attached?

We now have

sage: F = GF(13)
sage: F.coerce_map_from(F) is F.coerce_map_from(F)
True
sage: x = F(234234)
sage: %timeit t = F.coerce(x)
1000000 loops, best of 3: 862 ns per loop

and used to have

sage: F = GF(13)
sage: F.coerce_map_from(F) is F.coerce_map_from(F)
False
sage: x = F(234234)
sage: %timeit t = F.coerce(x)
10000 loops, best of 3: 132 us per loop

Apply trac4740-cache_coerce_from_self.patch​

[simon-king-jena]

comment:23

PS: The memleak in the ticket description has already been fixed elsewhere, this is now only about efficiency.

[simon-king-jena]

comment:24

It seems that the patchbot tries to apply both patches. Trying to get it right:

Apply trac4740-cache_coerce_from_self.patch​

[simon-king-jena]

comment:25

For the record: All tests pass, and I really don't see why the patchbot is trying to apply two patches.

[mwhansen]

comment:26

Your changes look good to me. I knew the patch I posted was probably dodgy, but you'd be the one who'd have the best grasp on it.

Thanks!

[mwhansen]

Reviewer: Mike Hansen

[jdemeyer]

Dependencies: #14519

[jdemeyer]

comment:28

This needs to be rebased to sage-5.12.beta0 + #14516 + #14519.

[simon-king-jena]

Attachment: trac4740-cache_coerce_from_self.patch.gz

Cache coercion from self to self

[simon-king-jena]

comment:29

Done! The only change concerns the new syntax for debug options. I guess I may revert to "positive review".

Apply trac4740-cache_coerce_from_self.patch​

[jdemeyer]

Merged: sage-5.12.beta3