memory allocated with `as_intArray` and `as_doubleArray` is never freed about swiglpk HOT 8 OPEN

There is a memory leak. Here's a python3 program that demonstrates it (replace gb = 16 with however much memory your computer has).

import numpy as np
import swiglpk as glpk

def alloc1mb():
    'allocates 1MB memory that should be freed when the function exits'

    size = 1024 * 1024 // 8 # 1 MB

    my_list = [0.0] * size

    arr = glpk.as_doubleArray(my_list) # runs out of memory
    #arr = np.array(my_list) # works fine
    #arr = glpk.doubleArray(size) # also works fine

def main():
    'main func'

    gb = 16 # how many GB to go through

    for k in range(1024 * gb):
        if k % 256 == 0:
            print("{:.2f} GB so far".format(k / 1024))
            
        alloc1mb()

if __name__ == "__main__":
    main()

If you watch the memory usage on your computer as it runs, you'll see memory quicky gets exhausted, whereas using doubleArray directly or np.array works fine (the memory gets freed after the function exits and can be reused).

Also, in the implementation of as_doubleArray and as_intArray, there's no checking if malloc fails, which it can.

from swiglpk.

Thank you for the report and the nice example @stanleybak. We have observed a leak on the optlang side before (see opencobra/optlang#125) but so far thought it was Python's fault for not cleaning up properly. This gives us another hint on how to resolve the issue.

I'm currently working on a major refactor of optlang, so if I can, I will also take this into account.

from swiglpk.

Here is a fixed implementation for as_doubleArray you can use in glpk.i (intArray will be similar). It creates a Python object from inside the C code, so that it will get cleaned up by Python's garbage collector when the object goes out of scope. Running it on the above program, the memory leak is no longer present. It's almost as fast as the existing implementation (I suspect the slight slowdown is due to the extra work in freeing the memory).

PyObject* as_doubleArray(PyObject *list)
{
    /* Check if is a list */
    if (!PyList_Check(list))
    {
        PyErr_SetString(PyExc_TypeError, "not a list");
        return NULL;
    }
    
    int size = PyList_Size(list);

    PyObject *pmod   = PyImport_ImportModule("swiglpk");

    if (!pmod)
    {
        PyErr_SetString(PyExc_ImportError, "swiglpk could not be imported");
        return NULL;
    }

    PyObject *pclass = PyObject_GetAttrString(pmod, "doubleArray");
    Py_DECREF(pmod);

    if (!pclass)
    {
        PyErr_SetString(PyExc_AttributeError, "swiglpk does not contain doubleArray");
        return NULL;
    }

    // call doubleArray constructor with size + 1
    PyObject *pargs  = Py_BuildValue("(i)", size + 1);

    if (!pargs)
    {
        Py_DECREF(pclass);
        PyErr_SetString(PyExc_RuntimeError, "building arguments list for doubleArray constructor failed");
        return NULL;
    }
    
    PyObject *pinst  = PyEval_CallObject(pclass, pargs);
    Py_DECREF(pclass);
    Py_DECREF(pargs);

    if (!pinst)
    {
        PyErr_SetString(PyExc_RuntimeError, "creating doubleArray failed");
        return NULL;
    }

    PyObject* pthis = PyObject_GetAttrString(pinst, "this");

    if (!pthis)
    {
        Py_DECREF(pinst);
        PyErr_SetString(PyExc_AttributeError, "doubleArray 'this' attribute not found");
        return NULL;
    }

    // convert 'this' to a c-style pointer
    doubleArray* double_arr = 0;
    int res = SWIG_ConvertPtr(pthis, (void**)&double_arr, SWIGTYPE_p_doubleArray,  0 );
    Py_DECREF(pthis);
        
    if (!SWIG_IsOK(res))
    {
        Py_DECREF(pinst);
        PyErr_SetString(PyExc_RuntimeError, "SWIG_ConvertPtr failed");
        return NULL;
    }

    for (int i=0; i<size; i++)
    {
        PyObject *o = PyList_GetItem(list, i);
        
        if (PyFloat_Check(o))
            double_arr[i+1] = PyFloat_AsDouble(o);
        else
        {
           Py_DECREF(pinst);
           PyErr_SetString(PyExc_TypeError, "list must contain floats");
           
           return NULL;
        }
    }

    // return the doubleArray instance
    return pinst;
}

from swiglpk.

Hmm, thank you for the effort. My only doubt here is that the bindings are, of course, auto-generated by SWIG. We do not manipulate them manually. We could but if possible I'd like to avoid that. We have been meaning to evaluate other bindings, though, such as CFFI.

from swiglpk.

My understanding was that the glpk.i file was manually written rather than auto-generated (is this not the case?). The only part I see dependent on swig is the object name (swiglpk.doubleArray), the constructor argument (array size), and the attribute this which is where swig stores the (wrapped) raw pointer.

We might be able to get around referring to this by instead calling doubleArray.cast(). Would that be better?

from swiglpk.

This seems super reasonable and we should include the proposed implementation.

from swiglpk.

Another approach I've been wondering about, what if optlang held internal numpy arrays and we use swig to copy the solution values into those arrays? That would avoid the leak, too, and might be faster since no new memory needs to be allocated.

from swiglpk.

In the glpk_interface? Yeah that makes sense. Though I wouldn't know how to cast from dictionary values directly to a numpy array without an intermediate list. The conversion here is pretty fast though since it all happens in C. It's a single allocation and then just copying of values, I think I benchmarked it initially and it was pretty negligible compared to all the optlang Python operations. We could add cast from numpy to the C arrays here though I wouldn't be sure how to go about it.

from swiglpk.