1

Dear stackoverflow community,

I am currently working on a MEX function that itself calls external C++ code. I am calling my MEX function in a double loop in matlab (48 x 48 times) creating a similarity matrix. The similarity is calculated by the forementioned MEX function.

/*
 * Matlab interface function.
 */
void mexFunction(int nlhs, mxArray *plhs[],
        int nrhs, const mxArray *prhs[]) {
    if (nrhs < 2) {
        //we need exactly two input strings.
        mexErrMsgTxt("Two input strings are needed!");
        return;
    }
    if (nrhs > 5) {
        mexErrMsgTxt("Maximum number of inputs is 5!");
        return;
    }
    if (nrhs == 3 || nrhs == 4) {
        mexErrMsgTxt("You are expected to give all three score vectors: for meta, middle and nodes score.");
        return;
    }
    if (nlhs != 1) {
        //and we only give one output argument
        mexErrMsgTxt("The fragment distance only provides one output argument!");
        return;
    }
    //if possible get score vectors
    if (nrhs == 5) {
        extractScoreVector(prhs, 2, META_SCORENUM, meta_scores);
        extractScoreVector(prhs, 3, MIDDLE_SCORENUM, middle_scores);
        extractScoreVector(prhs, 4, NODES_SCORENUM, nodes_scores);
    } else {
        //otherwise take default scores
        meta_scores[meta_del] = -1;
        meta_scores[meta_ins] = -1;
        meta_scores[meta_match] = 10;
        meta_scores[meta_mismatch] = -2;
        middle_scores[0] = -6;
        middle_scores[1] = -6;
        nodes_scores[nodes_del] = -18;
        nodes_scores[nodes_ins] = -18;
        nodes_scores[nodes_skipl] = 0;
        nodes_scores[nodes_skipr] = 0;
    }

    //get both string inputs.

    std::string firstSeq = getMatlabString(prhs, 0);
    std::string sndSeq = getMatlabString(prhs, 1);

    //split them into node encodings.
    firstNodes = split(firstSeq, '|');
    sndNodes = split(sndSeq, '|');
    //initialize distance table.
    distanceTable = (int**) malloc(sizeof (int *) * firstNodes.size());
    for (unsigned int i = 0; i < firstNodes.size(); i++) {
        distanceTable[i] = (int*) malloc(sizeof (int) * sndNodes.size());
        for (unsigned int j = 0; j < sndNodes.size(); j++) {
            distanceTable[i][j] = -1;
        }
    }

    //construct input for nodes alignment: nodes are only represented by index with normed length to 3 (instead of index 1 we append 001).
    std::stringstream nodesInput;

    //first the node indices of the first fragment.
    for (unsigned int i = 0; i < firstNodes.size(); i++) {
        int magnitude = getMagnitude(i);
        for (int j = 0; j < 3 - magnitude; j++) {
            nodesInput << '0';
        }
        nodesInput << i << '|';
    }
    //then an @
    nodesInput << '@';
    //then the reversed indices of the second fragment with normed length to 3 (instead of index 1 we append 001).
    for (int i = sndNodes.size() - 1; i >= 0; i--) {
        int magnitude = getMagnitude(i);
        for (int j = 0; j < 3 - magnitude; j++) {
            nodesInput << '0';
        }
        nodesInput << i << '|';
    }
    nodesInput << '\0';

    //call nodes alignment.

    char* nodes_argv[2];
    //fake program name, dummy string
    nodes_argv[0] = (char*) "nodes";
    //actual input. The stringstream string has to be bound to a constant string
    //reference in order to prevent damage to the string behind it. a string stream
    //usually only protects its memory until the string is first evaluated.
    //this special construct prevents the memory from being overwritten.
    const std::string& tmp = nodesInput.str();
    nodes_argv[1] = const_cast<char*> (tmp.c_str());

    //call nodes alignment.
    gapc::Opts opts;
    try {
        //parse inputs
        opts.parse(2, nodes_argv);
    } catch (std::exception &e) {
        std::cerr << "Exception: " << e.what() << '\n';
        std::exit(1);
    }
    nodes obj;

    try {
        obj.init(opts);
    } catch (std::exception &e) {
        std::cerr << "Exception: " << e.what() << '\n';
        std::exit(1);
    }

    obj.cyk();

    gapc::return_type res = obj.run();

    //free distance table memory.
    for (unsigned int i = 0; i < firstNodes.size(); i++) {
        free(distanceTable[i]);
    }
    free(distanceTable);

    //clear the node vectors
    firstNodes.clear();
    sndNodes.clear();

    //Version for simple score return value
    //plhs[0] = mxCreateDoubleScalar(res);

    //Version for string return value
    std::stringstream nodeOutput;
    obj.print_result(nodeOutput, res);
    const std::string& outStr = nodeOutput.str();
    plhs[0] = mxCreateString(outStr.c_str());
}

The external code is the gapc::opts and nodes obj part. Until now there are no known memory leak problems with the external code, so I am guessing that the problem is with me in the code I sent here. Unfortunately I am not able to find the mistake. I tried to free about any variable manually that is mentioned in the code, but this always lead to a MATLAB crash (As I see it Matlab tries to free the variables itself and crashes if there are not in memory anymore).

The memory leak is critical here: After about 7 steps in the loop there is already about 1 GB RAM occupied and it goes up to about 13 GB RAM in my test case. This is not plausible for the program so a memory leak seems very probable.

I also tried to find a fix in stackoverflow but everything mentioned here does not seem applicable to my scenario.

As the memory leak is very huge the variables that are most plausible (as they contain the most content) are firstSeq, sndSeq, firstNodes, sndNodes, distanceTable, opts and obj.

So my questions are:

  1. Have I forgotten to free one of those variables?
  2. Do you see something else that could cause a memory leak in the code?
  3. How can I fix that?

As far as my research goes the objects don't have to be freed as they are managed automatically. Still: Somewhere memory has to be leaking.

/edit

As requested I provide also the code of my helper functions. Please note that the functions "nodes_score", "meta_score" and "node_distance" are called from external functions that I call within the code using obj.run().

//using namespace std;

/*
 * This solution for the split problem is taken from
 * 
 * http://stackoverflow.com/questions/236129/splitting-a-string-in-c
 */
std::vector<std::string> &split(const std::string &s, char delim, std::vector<std::string> &elems) {
    std::stringstream ss(s);
    std::string item;
    while (std::getline(ss, item, delim)) {
        elems.push_back(item);
    }
    return elems;
}

std::vector<std::string> split(const std::string &s, char delim) {
    std::vector<std::string> elems;
    split(s, delim, elems);
    return elems;
}

//These vectors are global and contain the string encoding of the nodes for
//each fragment.
std::vector<std::string> firstNodes;
std::vector<std::string> sndNodes;
//this table contains the node distances for each combination of nodes.
int** distanceTable;

std::map<int, std::string> constructMetaMap(std::string nodeStr) {

    //get the single meta information strings
    std::vector<std::string> metaInfoStrs = split(nodeStr, '*');
    //initialize the map mapping meta information indices to the respective meta information content.
    std::map<int, std::string> metaMap;

    for (std::vector<std::string>::iterator metaInfoStr = metaInfoStrs.begin(); metaInfoStr != metaInfoStrs.end(); ++metaInfoStr) {
        //string stream for the meta info index.
        std::stringstream idxStream;
        int metaContentIdx = 1;
        for (std::string::iterator metaInfoChar = (*metaInfoStr).begin(); metaInfoChar != (*metaInfoStr).end(); ++metaInfoChar) {
            if (*metaInfoChar == '#') {
                //if we have finished looking for the current index, store the new map entry.
                int metaIdx;
                idxStream >> metaIdx;
                metaMap[metaIdx] = (*metaInfoStr).substr(metaContentIdx);
            } else {
                //otherwise store the current char and increment the start index of the actual meta info content.
                idxStream << *metaInfoChar;
                metaContentIdx++;
            }
        }
    }
    return metaMap;
}

const int MIDDLE_SCORENUM = 2;

int middle_scores[MIDDLE_SCORENUM];

/*
 * Emulates a call to meta alignment.
 * 
 * The node distance is defined as the sum over the distance between all meta
 * informations. If for a certain keyword no meta information exists in one of
 * the fragments a negative score is appended.
 */
int node_distance(unsigned int firstNodeIndex, unsigned int sndNodeIndex) {

    //check if the distance was already calculated.
    if (distanceTable[firstNodeIndex][sndNodeIndex] != -1) {
        return distanceTable[firstNodeIndex][sndNodeIndex];
    }

    //construct maps of keyword indices to meta information content.
    std::map<int, std::string> firstMetaMap = constructMetaMap(firstNodes[firstNodeIndex]);
    std::map<int, std::string> sndMetaMap = constructMetaMap(sndNodes[sndNodeIndex]);

    int node_distance_score = 0;
    //iterate over the first map.
    for (std::map<int, std::string>::const_iterator metaEntry = firstMetaMap.begin(); metaEntry != firstMetaMap.end(); ++metaEntry) {
        const int metaInfoIdx = metaEntry -> first;
        //if we don't have a value to that index in the second map, punish that.
        if (sndMetaMap.count(metaInfoIdx) == 0) {
            node_distance_score += middle_scores[0];
        } else {
            //otherwise do an alignment of the meta information.
            //and construct the input argument string array
            std::string sndMetaStr = sndMetaMap[metaInfoIdx];
            std::reverse(sndMetaStr.begin(), sndMetaStr.end());

            std::stringstream metaInput;
            metaInput << metaEntry -> second;
            metaInput << '@';
            metaInput << sndMetaStr;
            metaInput << '\0';

            char* argv[2];
            //fake program name, dummy string
            argv[0] = (char*) "meta";
            //actual input. The stringstream string has to be bound to a constant string
            //reference in order to prevent damage to the string behind it. a string stream
            //usually only protects its memory until the string is first evaluated.
            //this special construct prevents the memory from being overwritten.
            const std::string& tmp = metaInput.str();
            argv[1] = const_cast<char*> (tmp.c_str());

            //call meta alignment.
            gapc::Opts opts;
            try {
                opts.parse(2, argv);
            } catch (std::exception &e) {
                std::cerr << "Exception: " << e.what() << '\n';
                std::exit(1);
            }
            meta obj;

            try {
                obj.init(opts);
            } catch (std::exception &e) {
                std::cerr << "Exception: " << e.what() << '\n';
                std::exit(1);
            }
            gapc::add_event("start");

            obj.cyk();

            int metaScore = obj.run();
            node_distance_score += metaScore;
        }
    }
    //iterate over the second map
    for (std::map<int, std::string>::const_iterator metaEntry = sndMetaMap.begin(); metaEntry != sndMetaMap.end(); ++metaEntry) {
        const int metaInfoIdx = metaEntry -> first;
        //if we don't have a value to that index in the second map, punish that.
        if (firstMetaMap.count(metaInfoIdx) == 0) {
            node_distance_score += middle_scores[1];
        }
        //otherwise do nothing.
    }
    //store the result in the table.
    distanceTable[firstNodeIndex][sndNodeIndex] = node_distance_score;
    //clear the maps
    firstMetaMap.clear();
    sndMetaMap.clear();

    return node_distance_score;
}

const int META_SCORENUM = 6;
const int NODES_SCORENUM = 4;

int meta_scores[META_SCORENUM];
int nodes_scores[NODES_SCORENUM];

/*
 * Returns the score for a given operation
 */
int meta_score(meta_score_type type) {
    return meta_scores[(int) type];
}

/*
 * Returns the score for a given operation
 */
int nodes_score(nodes_score_type type) {
    return nodes_scores[(int) type];
}

// Utility function for extracting string inputs

std::string getMatlabString(const mxArray *prhs[], int strIndex) {
    const mxArray *strData = prhs[strIndex];
    int strLength = mxGetN(prhs[strIndex]) + 1;
    char *buf = mxArrayToString(strData);
    std::string s(buf);
    mxFree(buf);
    return s;
}

//Utility function for extracting the score vector.

void extractScoreVector(const mxArray *prhs[], int vecIdx, int scorelength, int scoreVec[]) {
    //Declarations
    const mxArray *vecData;
    double *singleVals;
    int rowLen, colLen;

    //Copy input pointer
    vecData = prhs[vecIdx];

    //Get matrix
    singleVals = (double*) mxGetPr(vecData);
    rowLen = mxGetN(vecData);
    colLen = mxGetM(vecData);

    //we don't care if it is a column or row vector but it has to be a
    //SCORENUM x 1 vector.
    if ((rowLen == 1 && colLen == scorelength) || (rowLen == scorelength && colLen == 1)) {
        for (int i = 0; i < scorelength; i++) {
            scoreVec[i] = (int) singleVals[i];
        }
    } else {
        mexErrMsgTxt("The score vector has the wrong number of entries!");
    }
}

int getMagnitude(int number) {
    if (number == 0) {
        return 1;
    }
    int magn = 0;
    while (number > 0) {
        magn++;
        number = number / 10;
    }
    return magn;
}
bpaassen
  • 31
  • 3
  • 2
    Do you need the memory for `distanceTable` to be allocated via `mxMalloc`? If not, try changing it to a `vector>`. Call `resize()` to set sizes, and when you're done swap with a temporary vector to free memory (`vector>().swap(distanceTable)`). Also, are you ever `clear()`ing the global vectors, and do you append to them on each call? You could try `clear mex_filename;` at the MATLAB command line between each call to the MEX fie to get MATLAB to unload it as an interim solution. – Praetorian Sep 18 '13 at 13:21
  • 1
    I dont see any obvious memory leaks. Assuming that your external C++ library is bug-free, you still haven't showed code for methods like `extractScoreVector`, `getMatlabString`, `split`. Maybe that's where the problem lies.. On another note, you should not use `cout` or `cerr` streams in MEX-files, use the provided `mexPrintf` instead, and use `mexErrMsgTxt` and the like to convey error exit code instead of `std::exit` – Amro Sep 18 '13 at 13:48
  • also why do you need to define the first three variables as globals? I see you are recomputing them each time the MEX-function is called.. – Amro Sep 18 '13 at 13:51
  • The variables have to be global because the called external program (obj.run()) calls a function in my code again that provides information based on those variables. I'll provide my full code in the main post. – bpaassen Sep 18 '13 at 14:40
  • @Praetorian: The clearing of the two global issues seems to help a little, but did not solve the issue. If I replace the matrix by the nested vector the memory explodes in the first calculation to 13 GB which my machine here can't handle. – bpaassen Sep 18 '13 at 14:41
  • 1
    Thank you for your quick replies! Also the clear mex_filename works. Thank you very much! Still I'll try to get rid of the memory leak. – bpaassen Sep 18 '13 at 14:46
  • `mxMalloc` and `mxFree` are slow. You can just use `malloc` and `free` (or `new` and `delete`). If you have no need to have this memory on managed by MATLAB, then don't use the mx API. I suspect that mxFree might be a bit "lazy" as MATLAB is with other memory operations. – chappjc Sep 18 '13 at 16:02
  • if `clear mymexfile` works, that would suggest that it is not a memory leak exactly, rather it is the global variables that are hogging memory. Those are the only ones not destructed between successive calls to the MEX-file. Try adding `firstNodes.clear(); sndNodes.clear();` towards the end when you no longer need them... – Amro Sep 18 '13 at 16:14
  • 1
    @Amro Calling `clear()` on a vector will not cause the vector to deallocate any memory. With C++11 you can call `vector::shrink_to_fit` but even that call is non-binding. About the only guaranteed way is to swap with a temporary vector. Anyway, there is a memory leak that I've posted in my answer. – Praetorian Sep 18 '13 at 16:27
  • @Praetorian: I see, thanks for the correction. And good catch with the `getMatlabString` leak +1. Perhaps you could also use [`mxArrayToString`](http://www.mathworks.com/help/matlab/apiref/mxarraytostring.html) to get the `char*` string, copy it into `std::string`, then `mxFree` it. – Amro Sep 18 '13 at 16:38
  • I've updated my answer using a `vector` instead of `unique_ptr`, but @Amro's version works just as well. I don't see any other leaks in your code. – Praetorian Sep 19 '13 at 16:17

1 Answers1

2

You're leaking memory in both the statements below:

//get both string inputs.
std::string firstSeq(getMatlabString(prhs, 0));
std::string sndSeq(getMatlabString(prhs, 1));

getMatlabString is allocating memory for the strings, and returning a pointer to the allocated memory. You're copying the contents of the string that pointer points to, but you never free the memory after that. This also explains why clear mex_filename; is fixing the issue, because MATLAB will automatically free memory allocated via mxMalloc and friends when the MEX file gets unloaded. I'd rewrite the getMatlabString function as

std::string getMatlabString(const mxArray *prhs[], int strIndex) {
    const mxArray *strData = prhs[strIndex];
    int strLength = mxGetN(prhs[strIndex]) + 1;
    std::unique_ptr<char[], void(*)(char *)> 
      buf( static_cast<char *>(mxCalloc(strLength, sizeof(char))),
           []( char *p ) { mxFree(p); } );

    mxGetString(strData, buf.get(), strLength);
    return std::string(buf.get());
}

If your compiler does not support unique_ptr and lambda expressions, you can replace that part with a vector<char>.

Also, as I mentioned in the comments I'd change distanceTable to vector<vector<int>> distanceTable;. Call vector::resize() to set the size to whatever you need, and if that variable must be global, swap with a temporary vector after you're done using it to free memory.

std::vector<std::vector<int>>().swap(distanceTable);

To use vector<char> above instead of unique_ptr

std::string getMatlabString(const mxArray *prhs[], int strIndex) {
    const mxArray *strData = prhs[strIndex];
    int strLength = mxGetN(prhs[strIndex]) + 1;
    std::vector<char> buf( strLength, 0 );

    mxGetString(strData, &buf[0], strLength);
    return std::string(&buf[0]);
}
Praetorian
  • 106,671
  • 19
  • 240
  • 328
  • From `mxFree` documentation: "By default, memory parcels created by mxCalloc, mxMalloc, and mxRealloc are nonpersistent. The memory management facility automatically frees all nonpersistent memory whenever a MEX-file completes. Thus, even if you do not call mxFree, MATLAB takes care of freeing the memory for you. Nevertheless, it is good programming practice to deallocate memory as soon as you are through using it." A MEX file does NOT need to be cleared from memory -- mexFunction just needs to return. Besides, these strings do not account for 1GB of RAM in 7 iterations. – chappjc Sep 18 '13 at 16:41
  • @chappjc: Note that it says "whenever a MEX *file* completes", and not "a MEX *function*" – Ben Voigt Sep 18 '13 at 16:42
  • That means returning to the MATLAB command prompt, not unloading the library. See the documentation for mexMakeArrayPersistent: "The MATLAB® memory management facility automatically frees a nonpersistent mxArray when the MEX-function finishes" – chappjc Sep 18 '13 at 16:44
  • @Praetorian: It seems crazy to use `mxCalloc` when you could just pass the `string`'s internal buffer (after setting the size). – Ben Voigt Sep 18 '13 at 16:44
  • @chappjc: From the question "I am calling my MEX function in a double loop in matlab" -- so you aren't returning to the command prompt – Ben Voigt Sep 18 '13 at 16:45
  • @chappjc: even if it does eventually reclaim memory, it can be really slow to rely on this automatic dellocation: http://stackoverflow.com/a/18662980/97160 – Amro Sep 18 '13 at 16:46
  • You are splitting hairs. I added the command prompt part as an example. It is when "the MEX-function finishes". Try it on your own! – chappjc Sep 18 '13 at 16:47
  • @Amro: I completely agree. I suggested to the OP in comments above to ditch the mx routines for that reason. Allowing MATLAB to manage memory unnecessarily is a bad idea. – chappjc Sep 18 '13 at 16:48
  • @chappjc: No, you didn't add the command prompt part. That's what [the official documentation](http://www.mathworks.com/help/matlab/apiref/mxfree.html) says, too. – Ben Voigt Sep 18 '13 at 16:50
  • @Ben Voigt: I stand corrected about the documentation. But going back to a script is effectively the same here. I still say try it. :/ – chappjc Sep 18 '13 at 16:52
  • 1
    fwiw, here is the function I usually use to convert an mxArray to a string http://pastebin.com/5yabNyMM: `std::string s = toString(prhs[0])` – Amro Sep 18 '13 at 16:57
  • @BenVoigt I agree, that is an annoying allocation to have to do. But resizing the string and writing to the internal buffer involves a `const_cast`, which *feels* wrong, although it's not an issue in this case, *if* the string is stored contiguously in memory. And that contiguous part is not mandated by C++03, so it could technically lead to UB. But then again, I've never heard of any string implementation that didn't have one single internal buffer. – Praetorian Sep 18 '13 at 17:02
  • 3
    @chappjc I've always felt that whole *mxMalloc allocated memory is automatically freed* part to be a little under-specified. Yes, MATLAB will probably collect the memory when the MEX *function* exits, but in practice it's not very useful because the automatic deallocation is pretty slow. OTOH, clearing the mex file unloads it from memory, and that does trigger a collection for whatever memory that mex file has allocated. – Praetorian Sep 18 '13 at 17:05
  • 1
    @Praetorian: No `const_cast` needed, just `&s[0]`. – Ben Voigt Sep 18 '13 at 18:00
  • @Praetorian: small typo, shouldn't it be: `std::unique_ptr buf(..)`? VC++ kept complaining about it with non-obvious error messages. – Amro Sep 19 '13 at 07:34
  • Thanks again a lot for your fast reactions. I replaced the mx functions for distanceTable as you recommended and used Amros suggestion for the conversion fucntion from mxArray to std::string. Finally I am using clear, too. It works but unfortunately there still seem to be memory leaks left. I was not able to implement your suggestion, Praetorian, my compiler throws errors and I do not quite understand how to replace the unique_ptr by vector as you suggested. The new code is in my original post. – bpaassen Sep 19 '13 at 09:18
  • @Amro When allocating an array, you want to use the `unique_ptr` partial specialization so `delete[]` is called instead of `delete`. With a custom deleter, it shouldn't make any difference as far as destruction goes, because our deleter will always do the right thing. But `unique_ptr` does provide `operator[]` which might be useful for indexing into the array. VS2010 doesn't implement implicit conversion of lambdas to function pointers, which could be the error you're seeing. Anyway, there's no need to involve `mxCalloc`. `unique_ptr buf(new char[strLength]);` works too. – Praetorian Sep 19 '13 at 16:12
  • @Praetorian: ah thanks again for the explanation! It was [telling](http://pastebin.com/gmJzM7EQ) me that I `cannot access private member declared in class std::unique_ptr<_Ty,_Dx>`, I'm thinking that it was somehow calling a private copy constructor? As for the function pointer I used `std::function` instead (VS2012 seems to accept the first as well, but still errors with the partial template specialization just like VS2010). Anyway I should have known it was a VC++ issue :) – Amro Sep 20 '13 at 04:47