Opened 7 years ago

Closed 7 years ago

#15493 closed defect (fixed)

qqbar.ANRoot creates tons of copies of interval fields

Reported by: mmezzarobba Owned by:
Priority: major Milestone: sage-6.3
Component: number fields Keywords: memory, qqbar
Cc: Merged in:
Authors: Marc Mezzarobba Reviewers: Vincent Delecroix, Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: b8b4265 (Commits) Commit: b8b426514bd28afe51ab863bf7d5cea1a77a9f91
Dependencies: Stopgaps:

Description (last modified by mmezzarobba)

sage.rings.qqbar.ANRoot._interval_fast() creates new interval fields by type(self._interval.parent())(prec), thus bypassing the cache provided by RealIntervalField(). Computations with algebraic numbers sometimes need gigabytes of memory to store these parents and the associated coercion structures.

Before (6.2.beta5): 

sage: set_random_seed(42)
sage: m = get_memory_usage()
sage: %time l = list(QQbar.random_element(poly_degree=3) == 0 for i in range(4000))
CPU times: user 10.7 s, sys: 24 ms, total: 10.7 s
Wall time: 10.7 s
sage: get_memory_usage(m)
7.77734375

With patch: 

sage: set_random_seed(42)
sage: m = get_memory_usage()
sage: %time l = list(QQbar.random_element(poly_degree=3) == 0 for i in range(4000))
CPU times: user 10.5 s, sys: 40 ms, total: 10.5 s
Wall time: 10.5 s
sage: get_memory_usage(m)
0.34375

Change History (20)

comment:1 Changed 7 years ago by mmezzarobba

  • Branch set to u/mmezzarobba/ticket/15493-memleak
  • Commit set to 17eece2087c144371dcf68ff7ebf2b74c690dba9
  • Status changed from new to needs_review

New commits:

​17eece2Fix memory leak in qqbar.ANRoot._interval_fast()

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

  • Authors set to Marc Mezzarobba
  • Status changed from needs_review to needs_work

Please use the correct doctest syntax: 

EXAMPLES::

    sage: 1 + 1
    2

More text::

    sage: 2 + 2
    4

comment:3 Changed 7 years ago by git

  • Commit changed from 17eece2087c144371dcf68ff7ebf2b74c690dba9 to 26079857fa777d2f7962a964dd95a667917800b2

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

​2607985Doctest syntax

comment:4 in reply to: ↑ 2 Changed 7 years ago by mmezzarobba

  • Status changed from needs_work to needs_review

Replying to jdemeyer:

Please use the correct doctest syntax:

Oops. Thanks!

comment:5 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.0 to sage-6.1

comment:6 Changed 7 years ago by mmezzarobba

Note that #14711 and #15367 likely avoid or at least mitigate the memory leak--but it does not hurt to fix interval_fast() in any case!

comment:7 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:8 Changed 7 years ago by mmezzarobba

  • Keywords memory qqbar added
  • Summary changed from Memory leak in QQbar to qqbar.ANRoot creates tons of copies of interval fields

comment:9 follow-up: Changed 7 years ago by zimmerma

I just tried your example with version 0.1 of ore_algebra (from the RISC homepage), but got an error: 

sage: rec.generalized_series_solutions(1, infolevel=5)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-4-ebd2bbf4a9be> in <module>()
----> 1 rec.generalized_series_solutions(Integer(1), infolevel=Integer(5))

TypeError: generalized_series_solutions() got an unexpected keyword argument 'infolevel'

Maybe you did use a more recent version of ore_algebra?

Paul

comment:10 in reply to: ↑ 9 Changed 7 years ago by mmezzarobba

Replying to zimmerma:

Maybe you did use a more recent version of ore_algebra?

It could be that I used the git version, yes. Does it work without infolevel?

comment:11 follow-up: Changed 7 years ago by zimmerma

Does it work without infolevel?

yes and no (with Sage 6.1 and ore_algebra 0.1): 

sage: rec.generalized_series_solutions(1)
...
ValueError: factorization of 0 not defined

Paul

comment:12 in reply to: ↑ 11 Changed 7 years ago by mmezzarobba

  • Description modified (diff)

Replying to zimmerma:

Does it work without infolevel?

yes and no (with Sage 6.1 and ore_algebra 0.1):

You are right, my example did not work with ore_algebra 0.1 (I used a development version that does not seem to be publicly available yet).

comment:13 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:14 follow-up: Changed 7 years ago by vdelecroix

Hi Marc,

To mention a trac ticket, use :trac:15493 instead of #15493. That way, there is a nice hyperlink in the documentation!

Once that done, it is ready for positive review.

Vincent

comment:15 Changed 7 years ago by vdelecroix

  • Status changed from needs_review to needs_work

comment:16 Changed 7 years ago by git

  • Commit changed from 26079857fa777d2f7962a964dd95a667917800b2 to b8b426514bd28afe51ab863bf7d5cea1a77a9f91

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

​b8b4265#ticket → :trac:`ticket`

comment:17 in reply to: ↑ 14 Changed 7 years ago by mmezzarobba

Replying to vdelecroix:

To mention a trac ticket, use :trac:15493 instead of #15493. That way, there is a nice hyperlink in the documentation!

Yes, I keep forgetting! Thanks!

comment:18 Changed 7 years ago by mmezzarobba

  • Status changed from needs_work to needs_review

comment:19 Changed 7 years ago by vdelecroix

  • Reviewers set to Vincent Delecroix, Jeroen Demeyer
  • Status changed from needs_review to positive_review

Hello,

Wonderful. Thanks.

Vincent

comment:20 Changed 7 years ago by vbraun

  • Branch changed from u/mmezzarobba/ticket/15493-memleak to b8b426514bd28afe51ab863bf7d5cea1a77a9f91
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.