Opened 9 years ago

Closed 9 years ago

#15284 closed defect (fixed)

Memory leak in the interface with Gurobi

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

Status badges

Description (last modified by Nathann Cohen)

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 9 years ago by Nathann Cohen

Branch: u/ncohen/15284
Commit: aa21671afc00003ed70095448146476eb036c143
Description: modified (diff)
Status: newneeds_review

New commits:

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

comment:2 Changed 9 years ago by Jernej Azarija

Hello!

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

comment:3 Changed 9 years ago by Nathann Cohen

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 9 years ago by git

Commit: aa21671afc00003ed70095448146476eb036c143012f44bcc29ee9fe36d7453ce6d1a7474c1bca31

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 9 years ago by Jernej Azarija

The patch looks good to me!

comment:6 Changed 9 years ago by Jernej Azarija

Status: needs_reviewpositive_review

The patch looks good to me!

comment:7 Changed 9 years ago by Nathann Cohen

Thaaaaaaaaaanks ! ;-)

Nathann

comment:8 Changed 9 years ago by Jeroen Demeyer

Milestone: sage-5.13sage-6.0

comment:9 Changed 9 years ago by For batch modifications

Milestone: sage-6.0sage-6.1

comment:10 Changed 9 years ago by Volker Braun

Reviewers: ​Jernej Azarija

comment:11 Changed 9 years ago by Volker Braun

Reviewers: ​Jernej AzarijaJernej Azarija

comment:12 Changed 9 years ago by Volker Braun

Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.