Opened 3 years ago

Closed 3 years ago

#22164 closed defect (fixed)

Memory leaks in eclib interface: includes eclib bugfix upgrade

Reported by: cremona Owned by:
Priority: major Milestone: sage-7.6
Component: interfaces Keywords:
Cc: Merged in:
Authors: John Cremona Reviewers: Jeroen Demeyer
Report Upstream: Fixed upstream, in a later stable release. Work issues:
Branch: 640fe83 (Commits) Commit: 640fe83501fd6c8837456405ffe2410848365463
Dependencies: Stopgaps:

Description (last modified by cremona)

As originally reported at https://ask.sagemath.org/question/36225/memory-leak-with-modular-symbols/

The interface from Sage to eclib introduces memory leaks since C++ objects are created but not destroyed. In all but one place there is an appropriate dealloc method, but not for class ECModularSymbol.

Here is a list, hopefully complete, all from sage.libs.eclib:

  • In homspace.pyx, the initializer for class ModularSymbols? calls new to create a C++ homspace, which is correctly deallocated
  • In mat.pyx, C++ matrices are created but again there is a dealloc to delete them.
  • In newforms.pyx, the initializer for class ECModularSymbol calls new newforms() and has no dealloc method.

This one is good:

  • In mwrank.pyx the initializer for class _Curvedata creates a C++ Curvedata via new which is correctly deallocated. Same for class _mw with a new nw. Same for class _two_descent and new two_descent.

In the course of fixing this some memory leaks were found in eclib resulting in a new version v20170122 which is at http://homepages.warwick.ac.uk/staff/J.E.Cremona/ftp/eclib-20170122.tar.bz2

Change History (78)

comment:1 follow-up: Changed 3 years ago by cremona

  • Dependencies set to #22077, #10256

I added #22077 and #10256 as dependencies since they make many changes in sage.libs.eclib, but as they have missed release 7.5 (I assume) someone might consider it urgent to fix this leak before 7.5 despite that.

comment:2 Changed 3 years ago by cremona

  • Description modified (diff)

comment:3 Changed 3 years ago by jdemeyer

  • Description modified (diff)

homespace? Sounds cool :-)

comment:4 in reply to: ↑ 1 Changed 3 years ago by jdemeyer

Replying to cremona:

I added #22077 and #10256 as dependencies since they make many changes in sage.libs.eclib, but as they have missed release 7.5 (I assume) someone might consider it urgent to fix this leak before 7.5 despite that.

As far as I know, the memory leak is not something new. If the problem has existed for a while and people didn't complain, I don't consider it very urgent.

comment:5 Changed 3 years ago by cremona

That is a fair point. The reason it came up now is that Rob Pollack and I were adding Iwasawa lambda- and mu-invariants to each of the elliptic curves in the LMFDB, which involves a one-off computation for each which uses this. Once we had tested the code Rob was intending to run it as far as practicable, and that will only be possible once this is fixed. And also, these computations will also use the additional functionality made available at #10256 and #22077.

comment:6 Changed 3 years ago by jdemeyer

Maybe we could discuss in Edinburgh?

comment:7 Changed 3 years ago by cremona

OK, let's. Meanwhile I am trying the same loop to 10000 as in the asksage post, and by 5000 it is using around 15g, so I stopped it and will try again with a small patch which adds a dealloc method.

comment:8 Changed 3 years ago by cremona

Report on test: after adding the dealloc method to the ECModularSYmbol class in the cython file newforms.pyx, and running the loop again, the memory usage is a lot less but it still sits at 13g at the end of the loop to 10000. There must be something else not getting cleared.

The exact test is

for N in range(1,10000):
     Cs = DB.isogeny_classes(N)
     for C in Cs:
         E=EllipticCurve(C[0][0])
         print(E.label())
         phi=E.modular_symbol()
Version 0, edited 3 years ago by cremona (next)

comment:9 Changed 3 years ago by cremona

  • Authors set to John Cremona
  • Branch set to u/cremona/22164
  • Commit set to c0306ee08b94061ef1f525cdc71d24c17e004ce3
  • Status changed from new to needs_review

New commits:

cb3fd05#22077 update eclib to v20170104
ee879eework in progress on modular symbols
0db2a00trac #10256: reviewer patches: first part, highlight wrong values, references
49be7d810256: fix one doctest
c0306ee#22164: add dealloc method to ECModularSymbol class in eclib interface

comment:10 Changed 3 years ago by cremona

NB only the last commit is new to this ticket, the others are from #10256 (and #22077).

comment:11 Changed 3 years ago by nbruin

I notice these lines in the init

        C = new Curve(a1,a2,a3,a4,a6)
        CD = new Curvedata(C[0],0)
        CR = new CurveRed(CD[0])

for which I cannot find explicit "del"s elsewhere. Do these get deleted? I think normally every "new" should be paired with a "del". It could be that your objects disappear into another object and that deallocating them is done as part of the deallocation of that object, but then your interface have that documented and you should only access that object via the encapsulating object.

comment:12 follow-up: Changed 3 years ago by cremona

I don't know the answer to that. Perhaps I should try deallocating those too and seeing if it helps. I did not write this wrapper and don't know cython wrapping at all well, but the difference is that thpse variables are local to the init function while the "new newforms..." is stored in self.

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

Replying to cremona:

I don't know the answer to that. Perhaps I should try deallocating those too and seeing if it helps. I did not write this wrapper and don't know cython wrapping at all well,

?? it's the C++ here (createfromcurve) that could do complicated stuff. That's yours, right?

but the difference is that thpse variables are local to the init function while the "new newforms..." is stored in self.

Cython wouldn't be able to generate "del"s for these automatically without extensive flow analysis, though: Just because the result of "new" gets assigned to a pointer variable that is local doesn't imply that the value doesn't get stored in other places as well.

In this case it does look like you should be able to at least do a del C; del CD; del CR without problem: their values are only used in dereferenced form C[0],CD[0],CR[0].

You would know if the routines CurveData,CurveRed,getconductor,createfromcurve do funny stuff (especially, if they take special measures such as "del"-ing their arguments!)

It does look like Tom must have been in a rather big hurry when he wrote this wrapper, or otherwise had a deep-rooted contempt for reusing memory.

comment:14 Changed 3 years ago by jdemeyer

As I mentioned on the mailing list, you don't need always need to use pointers. You could use just

cdef mytype obj = mytype(....)  # no del needed

instead of

cdef mytype* obj = new mytype(....)
del obj

This might not always work and might have performance implications, but it's at least something to try.

comment:15 follow-up: Changed 3 years ago by cremona

If I replace

cdef Curve *C

by

cdef Curve C

I get the error

sage/libs/eclib/newforms.pyx:181:19: C++ class must have a nullary constructor to be stack allocated

even though the C++ class Curve _does_ have a nullary constructor (see line 73 in https://github.com/JohnCremona/eclib/blob/master/libsrc/eclib/curve.h).

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

Replying to cremona:

even though the C++ class Curve _does_ have a nullary constructor (see line 73 in https://github.com/JohnCremona/eclib/blob/master/libsrc/eclib/curve.h).

It looks like this signature isn't known to cython though, since it doesn't seem to be exposed in the pxd file: https://git.sagemath.org/sage.git/tree/src/sage/libs/eclib/__init__.pxd?h=u/cremona/22164&id=c0306ee08b94061ef1f525cdc71d24c17e004ce3#n45

comment:17 Changed 3 years ago by jdemeyer

Right. Cython never reads any .h files, it just generates #include <foo.h> statements. Cython only reads the .pxd file and according to that, there is no nullary constructor. That's of course easily fixed by add a line Curve() in the cppclass Curve.

comment:18 Changed 3 years ago by cremona

Thanks for the tips. I am now trying the test again after adding those nullary constructors in the pxd file. Before that, with "del C" and 2 similar, it made little difference: at the end of the loop there was 13g in use according to "top".

There is probably a better way to see how much memory is in use, from within Sage itself. Can someone remind me?

comment:19 Changed 3 years ago by nbruin

When this was first reported I did look at gc.get_objects() and didn't find anything suspicious. So my guess is that the leaks are not happening on the python heap, but indeed (as indicated here) on some C++ or C heap. Valgrind would probably be your best bet.

comment:20 Changed 3 years ago by cremona

You are probably right. I had a masters student parallelise some of the linear algebra code 2 or 3 years ago (code that is running in this loop) and later found through valgrind that there was a memory leak introduced then. This is a real nuisance -- when eclib was first put into Sage I spent quite a while fixing all the valgrind issues, some of which were hard to track down. I spent a time last summer trying to find the problem here but did not succeed (see https://github.com/JohnCremona/eclib/issues/18).

I suppose this is a case of "reported upstream, waiting for a fix there".

comment:21 Changed 3 years ago by cremona

  • Report Upstream changed from N/A to Reported upstream. Developers acknowledge bug.

comment:22 follow-up: Changed 3 years ago by cremona

I am working on fixing the leak in eclib. Meanwhile it seems that wrapping the eclib call as follows

@fork
def work_on(C):
    E=EllipticCurve(C)
    print(E.label())
    phi=E.modular_symbol()
    sys.stdout.flush()
    
DB = CremonaDatabase()

for N in range(1,10000):
     Cs = DB.isogeny_classes(N)
     for C in Cs:
         work_on(C[0][0])

avoids eating up memory: on a test run, it has reached 8000 (out of 10000) and does not go over 200k according to "top".

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

Replying to cremona:

I see a research article coming out of this: Implementing a stack using "fork".

Last edited 3 years ago by nbruin (previous) (diff)

comment:24 in reply to: ↑ 23 ; follow-up: Changed 3 years ago by cremona

Replying to nbruin:

Replying to cremona:

I see a research article coming out of this: Implementing a stack using "fork".

Ha ha. If you want to be helpful, how about this: in line 96 of https://github.com/JohnCremona/eclib/blob/master/libsrc/arith.cc in a class destructor, there is a delete command. But according to valgrind the associated memory is not being deleted. So I added a print statement just before the delete, and now it does delete -- and valgrind is happy. But I cannot get this to work without adding a line which actually outputs something! This still happens enve with no optimzation (-O0).

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

Replying to cremona:

Ha ha. If you want to be helpful, how about this: in line 96 of https://github.com/JohnCremona/eclib/blob/master/libsrc/arith.cc in a class destructor, there is a delete command. But according to valgrind the associated memory is not being deleted. So I added a print statement just before the delete, and now it does delete -- and valgrind is happy.

It's hard to believe a C++ compiler would make an error with something that simple. Have you verified valgrind is right, i.e., if you run something along the lines:

    int i;
    primeclass *P;
    for ( i=0; i<1000000; i++){
        P = new primeclass;
        P->init(10000);
        delete P;
    };

do you see memory leaking? My guess would be that valgrind somehow misses the free ...

comment:26 Changed 3 years ago by cremona

There was a logical explanation (of course) but quite a strange one. I was running a test program from my test suite (as done by "make check") but running independently to use valgrind. Using the standard test input file. However for this test, the input file contains a filename and the program attempts to open that. In this scenario the file does not exist (the Makefile copies it from somewhere else) and the test program calls abort(). So while it is still strange that whether or not the destructor is called on abort() depends on there being code to produce output in the destructor, it was all a big red herring...

comment:27 Changed 3 years ago by cremona

I think I have fixed the memory leaks. Rather than open a new ticket for upgrading eclib again I'll do that on this ticket, and the test that it can be accepted should include running the loop given here without memory usage growing.

comment:28 Changed 3 years ago by git

  • Commit changed from c0306ee08b94061ef1f525cdc71d24c17e004ce3 to 290f0458f5cb8e3e676bd5e423dfe33f111b0d3a

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

cd09ec5Merge commit '49be7d8' into eclib20170122
03df05eminor change to eclib interface
b084ed2Merge branch '22164' into eclib20170122
290f045update eclib to v20170122 (memory leak fix)

comment:29 Changed 3 years ago by cremona

The branch u/cremona/22164 at commit 290f045 now includes (1) addition of a missing dealloc, (2) addition of null constructors for 3 eclib classes so pointers are not needed when constructing these, and (crucially) (3) a new eclib version which fixes the memory leak. It was also merged with 7.6.beta0.

The new eclib is at http://homepages.warwick.ac.uk/staff/J.E.Cremona/ftp/eclib-20170122.tar.bz2

There are no changes there except for the bug fix, so no chenges needed to Sage except the package version and checksum change.

I tested this with the original loop over conductors to 10000 and the memory in use was totally under control.

Please review!

comment:30 Changed 3 years ago by cremona

  • Report Upstream changed from Reported upstream. Developers acknowledge bug. to Fixed upstream, in a later stable release.
  • Summary changed from Memory leaks in eclib interface to Memory leaks in eclib interface: includes eclib bugfix upgrade

comment:31 Changed 3 years ago by jpflori

Maybe you could add a doctest to show the memory is now under control?

comment:32 Changed 3 years ago by cremona

That is a good idea. I will.

comment:33 Changed 3 years ago by cremona

Would something like this be OK? Before:

sage: get_memory_usage()
134024.41796875
sage: for E in cremona_curves([380,400]):
....:     M = E.modular_symbol()
sage: get_memory_usage()
134070.73046875

After:

sage: get_memory_usage()
136320.72265625
sage: for E in cremona_curves([380,400]):
....:     M = E.modular_symbol()
....:     
sage: get_memory_usage()
136322.546875

Anything showing a greater leakage will take longer (the above is just a few seconds). If the run goes from 11 to 400 instead then the usage goes up by 49 before and 2 after.

comment:34 Changed 3 years ago by nbruin

So the test should probably be something like

T=get_memory_usage()
for E in cremona_curves([380,400]):
    M = E.modular_symbol()
assert get_memory_usage(T) < 4

The value you're getting out of this will be very much dependent on the weather and the architecture you're running on, so you have to be a bit conservative in test failure to avoid too many false positives.

comment:35 Changed 3 years ago by jdemeyer

I remember discussing this for another application.

The best way to check that foo() does not leak memory is to run foo() a number of times and record the memory usage mi after each call of foo(). There is no memory leak if, for some index i, we have mi+j ≤ mi for all j = 1, ..., N (for some suitable fixed value of N like N=10).

Put in a different way: there is no memory leak if you are able to run foo() N times where the memory never increases beyond the initial memory usage, but allowing for a few initial calls of foo() where the memory is allowed to increase.

Last edited 3 years ago by jdemeyer (previous) (diff)

comment:36 Changed 3 years ago by cremona

The trouble with using this general strategy is that elliptic curves are cached, so something like

sage: get_memory_usage()
66277.4609375
sage: for _ in range(100):
....:     M = EllipticCurve('90a1').modular_symbol()
....:     
sage: get_memory_usage()
66277.4609375

shows nothing useful (in this run I had already created the modular symbol for this curve). However one could use:

sage: from sage.libs.eclib.newforms import ECModularSymbol

directly. Testing this I get the impression that there is a speed regression in the new version which needs to be looked into. If you see the same, this will have to wait, and will be several days before I can work on it further.

comment:37 Changed 3 years ago by cremona

There is a speed regression compared with the version in 7.5 but I think this is already true for the eclib version already merged into 7.6, and it has a reason: to get the normalization right for modular symbols (which is what that eclib upgrade was all about) one has to compute some periods to a certain precision, which requires computing a few hundred ap and then doing the numerical integration (summing series). So there was a price to pay for getting the normalization right, and that should not stop this bug-fix leak fix going in.

I don't have time to provide more exact data now.

comment:38 Changed 3 years ago by jdemeyer

You can use

EllipticCurve('90a1').modular_symbol.clear_cache()

to clear the cache.

comment:39 Changed 3 years ago by cremona

  • Status changed from needs_review to needs_work

I'm setting back to Needs Work while I add a test to show that the leak is fixed.

comment:40 Changed 3 years ago by cremona

OK, so I am adding this test:

sage: from sage.libs.eclib.newforms import ECModularSymbol
sage: E = EllipticCurve([1,1,0,-108,-432]) # conductor 930
sage: mem = get_memory_usage()
sage: for _ in range(10): M = ECModularSymbol(E)
sage: mem2 = get_memory_usage()
sage: mem2-mem # random
3.34375

where the above shows a leak of 3.3MB before; after, it is 0.0.

comment:41 Changed 3 years ago by git

  • Commit changed from 290f0458f5cb8e3e676bd5e423dfe33f111b0d3a to 193620adad64103ae7c4d09b89a1c0a7a9bae8cb

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

193620a#22164 added test to show leak fixed

comment:42 Changed 3 years ago by cremona

  • Status changed from needs_work to needs_review

comment:43 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to needs_work

First of all, the branch needs to be rebased.

Second, I'm not totally convinced by the memleak test code: the mem2 - mem < 1 seems rather arbitrary. Although, if it passes doctests, I can accept it.

Still, I suggest to change

        sage: mem2-mem # random
        0.75
        sage: assert(mem2-mem < 1)

to

        sage: (mem2 - mem < 1) or (mem2 - mem)
        True

This use of or in doctests is a neat trick I learned from Robert Bradshaw: in case of failure, you will see the actual value of mem2 - mem.

comment:44 Changed 3 years ago by cremona

I'll make the suggested changes. I tested on more than one machine and found the exact difference mem2-mem was not the same in all runs. It is certainly less than before though! And certainly, the problem is much more severe after running several large examples, but these take too long for a doctest.

comment:45 Changed 3 years ago by cremona

I hope you will be happy with a merge of develop into my branch rather than a literal rebase.

comment:46 Changed 3 years ago by jdemeyer

Absolutely.

comment:47 Changed 3 years ago by git

  • Commit changed from 193620adad64103ae7c4d09b89a1c0a7a9bae8cb to d870d867b18c132895ce009d1021f30ad6a26b7e

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

d870d86Merge branch 'develop' into eclib20170122

comment:48 Changed 3 years ago by cremona

  • Status changed from needs_work to needs_review

OK, try this.

comment:49 Changed 3 years ago by jdemeyer

  • Dependencies #22077, #10256 deleted

comment:50 Changed 3 years ago by jdemeyer

  • Milestone changed from sage-7.5 to sage-7.6
  • Status changed from needs_review to needs_info

Where are the eclib sources? Please put an obvious link in the ticket description.

comment:51 Changed 3 years ago by cremona

  • Description modified (diff)
  • Status changed from needs_info to needs_review

Sorry!

comment:52 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Please add a doctest

sage: ECModularSymbol.__new__(ECModularSymbol)
Modular symbol with sign 0 over Rational Field attached to None

to show that this doesn't crash Sage.

The reason I ask this is that a non-trivial __dealloc__ has potential to crash Sage if it makes assumptions which are not satisfied after __new__.

Here, there is no problem because Cython initializes self.nfs = NULL in __new__ and deleting a NULL pointer is safe.

Last edited 3 years ago by jdemeyer (previous) (diff)

comment:53 Changed 3 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer

Apart from the doctest for __new__, this ticket looks good.

comment:54 Changed 3 years ago by git

  • Commit changed from d870d867b18c132895ce009d1021f30ad6a26b7e to 9ac02b5fd3dbb0252c6d96bd16be9fadedc76c8a

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

9ac02b5#22164 one more doctest after review

comment:55 Changed 3 years ago by cremona

OK done. I would never have thought of that...

comment:56 Changed 3 years ago by cremona

  • Status changed from needs_work to needs_review

comment:57 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to positive_review

comment:58 Changed 3 years ago by vbraun

  • Status changed from positive_review to needs_work

On OSX I get:

sage -t --long src/sage/libs/eclib/newforms.pyx
4157**********************************************************************
4158File "src/sage/libs/eclib/newforms.pyx", line 135, in sage.libs.eclib.newforms.ECModularSymbol
4159Failed example:
4160    (mem2-mem < 1) or (mem2 - mem)
4161Expected:
4162    True
4163Got:
4164    2.0
4165**********************************************************************
41661 item had failures:
4167   1 of  41 in sage.libs.eclib.newforms.ECModularSymbol
4168    [96 tests, 1 failure, 60.32 s]
4169

comment:59 Changed 3 years ago by jdemeyer

I was afraid that something like this might happen. I suggest to either remove the test or implement 35.

comment:60 follow-up: Changed 3 years ago by cremona

I have no way to test on OSX. This number is going to vary a lot from machine to machine. I only put in a test like this because an earlier reviewer wanted me to. there is no significance at all to the memory difference except that it is less than with the old code, and there is no way to test that!

I was going to suggest just removing this test, but I see that Jeroen is suggesting that his earlier proposal is implemented. Well, I am not going to promise to do that since I am busy and it already took several days work actually fixing some leaks.

comment:61 in reply to: ↑ 60 Changed 3 years ago by jdemeyer

Replying to cremona:

Jeroen is suggesting that his earlier proposal is implemented.

Please read my comment again carefully :-)

comment:62 Changed 3 years ago by cremona

Yes, I spotted that. I am not sure that Volker would be happy to have no doctest?

In fact a task I had to do today took less time than I expected so I can have a go at implementing the suggestion at 35 above.

comment:63 Changed 3 years ago by cremona

This looks good. AFter

sage: E = EllipticCurve([1,1,0,-108,-432])
sage: def foo():
....:     M = ECModularSymbol(E)
sage: from sage.libs.eclib.newforms import ECModularSymbol

we have

sage: print(get_memory_usage())
136409.183594
sage: for _ in range(30):
....:     foo()
....:     print(get_memory_usage())
....:     
136409.367188
136409.367188
136409.367188
136409.367188
136409.367188
136409.367188
136409.367188
136409.367188
136409.367188

etc ad infinitum (or 30, whichever comes first). This is a much better test since on anothe rmachine without the new version it looks like

sage: print(get_memory_usage())
66302.7304688
sage: for _ in range(30):
....:     foo()
....:     print(get_memory_usage())
....:     
66303.03125
66303.3242188
66303.6171875
66303.9492188
66304.2851562
66304.6210938
66304.9570312
66305.2929688
66305.6289062
66305.9648438
66306.3007812
66306.6328125
66306.96875
66307.3046875
66307.640625
66307.9765625
66308.3125
66308.6484375
66308.9804688
66309.3164062
66309.6523438
66309.9882812
66310.3242188
66310.6601562
66310.9960938
66311.3320312
66311.6640625
66312.0
66312.3359375
66312.671875

This suggest that in the doctest I should compare the memory usage after 1 and 10 calls rather than 0 and 10 as it is now.

comment:64 Changed 3 years ago by git

  • Commit changed from 9ac02b5fd3dbb0252c6d96bd16be9fadedc76c8a to eeeca4e18188129045fd5ca5bfa1df8bcffad293

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

eeeca4e#22164 improve doctest

comment:65 Changed 3 years ago by cremona

  • Status changed from needs_work to needs_review

OK, here's a new version of the doctest. It calls ECModularSymbol() twice before recording the first memory usage, then (as before) 10 more times before taking the second reading. And to avoid comments about arbitrariness I test these for equality -- which works for me but is possibly too strict.

Please test again.

comment:66 Changed 3 years ago by cremona

ping!

comment:67 Changed 3 years ago by jdemeyer

Somehow I thought this ticket was already finished...

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

  • Status changed from needs_review to needs_work

You should mark all those memory tests as # long time.

comment:69 in reply to: ↑ 68 ; follow-up: Changed 3 years ago by cremona

Replying to jdemeyer:

You should mark all those memory tests as # long time.

The lines which take a long time are tagged # long time already! So I suppose you mean the lines which call get_memory_usage() too? That is hardly necessary, is it?

Last edited 3 years ago by cremona (previous) (diff)

comment:70 in reply to: ↑ 69 Changed 3 years ago by jdemeyer

Replying to cremona:

Replying to jdemeyer:

You should mark all those memory tests as # long time.

The lines which take a long time are tagged # long time already! So I suppose you mean the lines which call get_memory_usage() too?

Yes, I mostly mean the final line (mem2==mem) or (mem2 - mem)

That is hardly necessary, is it?

Technically you are right, but it looks strange to me to run the memory tests when you are not calling ECModularSymbol(E). It is actually a bit dangerous since even running mem = get_memory_usage() might change the amount of memory allocated. So it would give me peace of mind to mark those tests as # long time too.

comment:71 Changed 3 years ago by git

  • Commit changed from eeeca4e18188129045fd5ca5bfa1df8bcffad293 to 40684b9c5beb99f5073eacc1d1cc323b007c623c

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

40684b9#22164 added missing # long time tags

comment:72 Changed 3 years ago by cremona

  • Status changed from needs_work to needs_review

Done


New commits:

40684b9#22164 added missing # long time tags

comment:73 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Sorry to spoil the party, but this regularly fails on OS X:

sage -t --long --warn-long 145.1 src/sage/libs/eclib/newforms.pyx
**********************************************************************
File "src/sage/libs/eclib/newforms.pyx", line 137, in sage.libs.eclib.newforms.ECModularSymbol
Failed example:
    (mem2==mem) or (mem2 - mem)                 # long time
Expected:
    True
Got:
    1.0
**********************************************************************
1 item had failures:
   1 of  42 in sage.libs.eclib.newforms.ECModularSymbol
    [97 tests, 1 failure, 41.65 s]
sage -t --long --warn-long 145.1 src/sage/libs/eclib/newforms.pyx
    [97 tests, 41.31 s]
sage -t --long --warn-long 145.1 src/sage/libs/eclib/newforms.pyx
    [97 tests, 41.64 s]
sage -t --long --warn-long 145.1 src/sage/libs/eclib/newforms.pyx
**********************************************************************
File "src/sage/libs/eclib/newforms.pyx", line 137, in sage.libs.eclib.newforms.ECModularSymbol
Failed example:
    (mem2==mem) or (mem2 - mem)                 # long time
Expected:
    True
Got:
    2.0
**********************************************************************
1 item had failures:
   1 of  42 in sage.libs.eclib.newforms.ECModularSymbol
    [97 tests, 1 failure, 41.72 s]
sage -t --long --warn-long 145.1 src/sage/libs/eclib/newforms.pyx
**********************************************************************
File "src/sage/libs/eclib/newforms.pyx", line 137, in sage.libs.eclib.newforms.ECModularSymbol
Failed example:
    (mem2==mem) or (mem2 - mem)                 # long time
Expected:
    True
Got:
    17.0
**********************************************************************
1 item had failures:
   1 of  42 in sage.libs.eclib.newforms.ECModularSymbol
    [97 tests, 1 failure, 42.46 s]

comment:74 follow-up: Changed 3 years ago by cremona

What a pain. I don't use Macs. I do not test eclib on Macs. If someone told me tomorrow that eclib does no longer work on some Mac system I would do absolutely nothing about it except possibly to put a warning into its documentation and wait for someone else to fix it. Ditto cygwin. Ditto clang, whatever that is.

It took quite a long time to track down the leak and fix it. It has taken far longer to write a doctest which keeps people happier -- still not finished with that. It is *obvious* that the memory used will be machine dependent, hance *stupid* to have a doctest which relies on some measure of memory. The only sensible measure is to compare the amount of memroy in use before and after the patch, one the same machine, and the doctesting system cannot do that.

Please can we delete this doctest entirely? It is in any case not testing that part of Sage works, only that part of one of its dependencies does. If that would get this into 7.6 that would be worth something.

I will delete the doctest. That is absolutely the last contribution I will make to this ticket.

comment:75 Changed 3 years ago by git

  • Commit changed from 40684b9c5beb99f5073eacc1d1cc323b007c623c to 640fe83501fd6c8837456405ffe2410848365463

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

640fe83#22164 deleted machine-dependent doctest

comment:76 in reply to: ↑ 74 Changed 3 years ago by jdemeyer

Replying to cremona:

The only sensible measure is to compare the amount of memroy in use before and after the patch, one the same machine

That's not true. There are robust ways to test memory leaks, see 35. Of course, it's more work, but it can be done. Maybe we should have some Sage infrastructure for this, say test_memory_leak(func) for a function func.

I will delete the doctest.

Fine for me. It will allow this ticket to finally move forward.

comment:77 Changed 3 years ago by jdemeyer

  • Status changed from needs_work to positive_review

comment:78 Changed 3 years ago by vbraun

  • Branch changed from u/cremona/22164 to 640fe83501fd6c8837456405ffe2410848365463
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.