Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#11917 closed defect (fixed)

Memory leaks with LP Solvers

Reported by: ncohen Owned by: tbd
Priority: major Milestone: sage-4.8
Component: linear programming Keywords:
Cc: Merged in: sage-4.8.alpha0
Authors: Nathann Cohen Reviewers: Peter Müller, Paul Zimmermann
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #11588 Stopgaps:

Description (last modified by leif)

As reported on sage-support, there is(was) something dead wrong with the LP classes : they did not free some of the memory they used.

This patch solves it !

Nathann

Reported by Peter Mueller on sage-support.


Apply: trac_11917.patch

(Based on Sage 4.7.2.alpha3 / #11588.)

Attachments (1)

trac_11917.patch (4.4 KB) - added by ncohen 8 years ago.

Download all attachments as: .zip

Change History (21)

Changed 8 years ago by ncohen

comment:1 Changed 8 years ago by ncohen

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

comment:2 Changed 8 years ago by ncohen

  • Component changed from PLEASE CHANGE to linear programming

comment:3 Changed 8 years ago by zimmerma

  • Type changed from PLEASE CHANGE to defect

comment:4 follow-up: Changed 8 years ago by zimmerma

  • Reviewers set to Paul Zimmermann
  • Status changed from needs_review to needs_work
  • Work issues set to patch fails to apply

the patch fails to apply to 4.7.1:

----------------------------------------------------------------------
| Sage Version 4.7.1, Release Date: 2011-08-11                       |
| Type notebook() for the GUI, and license() for information.        |
----------------------------------------------------------------------
Loading Sage library. Current Mercurial branch is: 11917
sage: hg_sage.import_patch("/tmp/trac_11917.patch")
cd "/usr/local/sage-4.7.1/sage/devel/sage" && hg status
cd "/usr/local/sage-4.7.1/sage/devel/sage" && hg status
cd "/usr/local/sage-4.7.1/sage/devel/sage" && hg import   "/tmp/trac_11917.patch"
applying /tmp/trac_11917.patch
patching file sage/numerical/backends/coin_backend.pxd
Hunk #1 FAILED at 97
1 out of 1 hunks FAILED -- saving rejects to file sage/numerical/backends/coin_backend.pxd.rej
patching file sage/numerical/backends/coin_backend.pyx
Hunk #3 FAILED at 986
1 out of 3 hunks FAILED -- saving rejects to file sage/numerical/backends/coin_backend.pyx.rej
abort: patch failed to apply

Paul

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

Replying to zimmerma:

the patch fails to apply to 4.7.1:

That's most probably due to #11588, merged into Sage 4.7.2.alpha3.

comment:6 in reply to: ↑ 5 Changed 8 years ago by zimmerma

That's most probably due to #11588, merged into Sage 4.7.2.alpha3.

if so it should be clearly indicated to which version the patch applies.

Paul

comment:7 Changed 8 years ago by leif

  • Dependencies set to #11588
  • Description modified (diff)
  • Status changed from needs_work to needs_review
  • Work issues patch fails to apply deleted

Patch also applies clean to Sage 4.7.2.alpha4.

IMHO all patches should in general be based on the latest devel release (or at least still apply to it); an attachment comment stating on which version a patch is based is of course not bad, especially as time goes by.

I really don't understand why the patchbot still uses 4.7.1; as such, it is almost useless. Worse, it reports failures for patches that apply (and work) well. (And often hangs such that the HTTP connections for trac pages are kept open...)

comment:8 Changed 8 years ago by leif

  • Description modified (diff)

comment:9 Changed 8 years ago by leif

Passes all (long) tests in sage/numerical/, and Peter Mueller's example (at first glance) doesn't eat up memory as it did before.

Haven't looked at the code though.

comment:10 Changed 8 years ago by pmueller

  • Status changed from needs_review to positive_review

The patch installed fine in sage-4.7.2alpha, and passed all the tests in sage/numerical/

I ran my original program again which goes through thousands of different small ilp problems. The memory usage was basically constant, so the memory leak apparently is fixed.

As this patch just fixed a bug and didn't add new features, I guess it is save to close the ticket.

  • Peter Mueller

comment:11 Changed 8 years ago by ncohen

  • Reviewers changed from Paul Zimmermann to Peter Mueller, Paul Zimmermann

Thaaaaaaaaaaaaaaaaaanks ! :-)

Nathann

comment:12 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-4.7.2 to sage-4.7.3

comment:13 follow-up: Changed 8 years ago by zimmerma

why did the milestone change? Is 4.7.2 already out? I don't see it on sagemath.org.

Paul

comment:14 in reply to: ↑ 13 ; follow-up: Changed 8 years ago by jdemeyer

Replying to zimmerma:

Is 4.7.2 already out?

Not yet, but it should be almost out. A release candidate is in preparation.

comment:15 in reply to: ↑ 14 Changed 8 years ago by leif

Replying to jdemeyer:

Replying to zimmerma:

Is 4.7.2 already out?

Not yet, but it should be almost out. A release candidate is in preparation.

So a small patch only fixing a memory leak isn't worth getting into the final?

comment:16 Changed 8 years ago by jdemeyer

  • Merged in set to sage-4.7.3.alpha0
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:17 Changed 8 years ago by zimmerma

I wonder why you already work on sage-4.7.3, since 4.7.2 is not yet available (even in source form) on sagemath.org...

Paul

comment:18 Changed 8 years ago by jdemeyer

  • Milestone sage-4.7.3 deleted

Milestone sage-4.7.3 deleted

comment:19 Changed 8 years ago by jdemeyer

  • Merged in changed from sage-4.7.3.alpha0 to sage-4.8.alpha0
  • Milestone set to sage-4.8

comment:20 Changed 8 years ago by jdemeyer

  • Reviewers changed from Peter Mueller, Paul Zimmermann to Peter Müller, Paul Zimmermann
Note: See TracTickets for help on using tickets.