Opened 6 years ago

Closed 6 years ago

#15284 closed defect (fixed)

Memory leak in the interface with Gurobi

Reported by: ncohen Owned by:
Priority: major Milestone: sage-6.1
Component: linear programming Keywords:
Cc: azi Merged in:
Authors: Nathann Cohen Reviewers: Jernej Azarija
Report Upstream: N/A Work issues:
Branch: u/ncohen/15284 (Commits) Commit: 012f44bcc29ee9fe36d7453ce6d1a7474c1bca31
Dependencies: Stopgaps:

Description (last modified by ncohen)

As reported by Jernej, the GRBenv ** env structure is allocated but not freed on __dealloc___. That's fixed here !

There was also a problem with the .copy() function of gurobi which should have been cpdef. It wasn't, and doctests did not pass. I don't get how we missed that :-/

Nathann

Change History (12)

comment:1 Changed 6 years ago by ncohen

  • Branch set to u/ncohen/15284
  • Commit set to aa21671afc00003ed70095448146476eb036c143
  • Description modified (diff)
  • Status changed from new to needs_review

New commits:

[changeset:aa21671]Memory leak in the interface with Gurobi, and broken doctest

comment:2 Changed 6 years ago by azi

Hello!

The patch is good just a short question - why do we need to check if it is NULL?

comment:3 Changed 6 years ago by ncohen

Ahahaha. Don't really know. Just used to checking that before freeing memory :-)

This being said, I just checled the doc and it explicitly says that env should only be freed after the model has been freed : http://www.gurobi.com/documentation/5.6/reference-manual/c_grbfreeenv

Also, there was some "weird" memory leak, which this patch now fixes : in order to define a model you need to define a "master environment", and the model creates a copy of the "master environment", and that copy is the one you should work on. However, if you delete the master environment segfault follows.

So. This patch creates a "master environment" variable to STORE the temporary environment that was never deallocated. Everything then is done on the real environment, and when everything is done both environment are deallocated. Without checks for NULL, as it has been checked before, when the environment were created.

Weird behaviour, but branch updated ! ;-)

Nathann

P.S. : Oh, and I also replace ** model by * model. Makes the code simpler :-P

comment:4 Changed 6 years ago by git

  • Commit changed from aa21671afc00003ed70095448146476eb036c143 to 012f44bcc29ee9fe36d7453ce6d1a7474c1bca31

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

[changeset:012f44b]Memory leak in the interface with Gurobi, and broken doctest

comment:5 Changed 6 years ago by azi

The patch looks good to me!

comment:6 Changed 6 years ago by azi

  • Status changed from needs_review to positive_review

The patch looks good to me!

comment:7 Changed 6 years ago by ncohen

Thaaaaaaaaaanks ! ;-)

Nathann

comment:8 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-5.13 to sage-6.0

comment:9 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.0 to sage-6.1

comment:10 Changed 6 years ago by vbraun

  • Reviewers set to ​Jernej Azarija

comment:11 Changed 6 years ago by vbraun

  • Reviewers changed from ​Jernej Azarija to Jernej Azarija

comment:12 Changed 6 years ago by vbraun

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