Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Memory leak in the interface with Gurobi #15284

Closed
nathanncohen mannequin opened this issue Oct 16, 2013 · 13 comments
Closed

Memory leak in the interface with Gurobi #15284

nathanncohen mannequin opened this issue Oct 16, 2013 · 13 comments

Comments

@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented Oct 16, 2013

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

CC: @sagetrac-azi

Component: linear programming

Author: Nathann Cohen

Branch/Commit: u/ncohen/15284 @ 012f44b

Reviewer: Jernej Azarija

Issue created by migration from https://trac.sagemath.org/ticket/15284

@nathanncohen nathanncohen mannequin added this to the sage-5.13 milestone Oct 16, 2013
@nathanncohen

This comment has been minimized.

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Oct 16, 2013

New commits:

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

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Oct 16, 2013

Branch: u/ncohen/15284

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Oct 16, 2013

Commit: aa21671

@nathanncohen nathanncohen mannequin added the s: needs review label Oct 16, 2013
@sagetrac-azi
Copy link
Mannequin

sagetrac-azi mannequin commented Oct 16, 2013

comment:2

Hello!

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

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Oct 17, 2013

comment:3

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 17, 2013

Changed commit from aa21671 to 012f44b

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 17, 2013

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

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

@sagetrac-azi
Copy link
Mannequin

sagetrac-azi mannequin commented Oct 18, 2013

comment:5

The patch looks good to me!

@sagetrac-azi
Copy link
Mannequin

sagetrac-azi mannequin commented Oct 18, 2013

comment:6

The patch looks good to me!

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Oct 18, 2013

comment:7

Thaaaaaaaaaanks ! ;-)

Nathann

@jdemeyer jdemeyer modified the milestones: sage-5.13, sage-6.0 Oct 19, 2013
@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.0, sage-6.1 Dec 17, 2013
@vbraun
Copy link
Member

vbraun commented Dec 21, 2013

Reviewer: ​Jernej Azarija

@vbraun
Copy link
Member

vbraun commented Dec 21, 2013

Changed reviewer from ​Jernej Azarija to Jernej Azarija

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants