Memory leak while iterating over commits and their diffs?

[jfremy]

I have some code that iterates over a range of commits and tries to extract some information from each commit (which requires to look at the diff of each commit). For longer histories, this systematically ends up with a process out memory error or with a core dump.

I made a modified examples program that triggers it (just put it in nodegit examples directory and run it).
It will iterate over the commits and trigger the error.

Here's the code:

var nodegit = require("../");
var path = require("path");

var repo;

function process(commit) {
  "use strict";
  console.log("commit " + commit.sha());
  return commit.getDiff().then(function (diffList) {
    return Promise.all(diffList.map(function (diff) {
      return diff.patches().then(function (patches) {
        return Promise.all(patches.map(function (patch) {
          return patch.hunks().then(function (hunks) {
            return Promise.all(hunks.map(function (hunk) {
              return hunk.lines().then(function (lines) {
                return true;
              });
            }));
          });
        }));
      })
    })).then(function () {
      if(commit.parentcount() === 0) {
        return false;
      } else {
        return repo.getCommit(commit.parentId(0));
      }
    });
  });
}

function iterate(commit) {
  "use strict";

  return process(commit).then(function(childcommit) {
    if(!childcommit) {
      return true;
    } else {
      return iterate(childcommit);
    }
  });
}

// This code examines the diffs between a particular commit and all of its
// parents. Since this commit is not a merge, it only has one parent. This is
// similar to doing `git show`.

nodegit.Repository.open(path.resolve(__dirname, "../.git"))
  .then(function (re) {
    repo = re;
    return repo.getHeadCommit();
  })
  .then(function (commit) {
    return iterate(commit);
  })
  .done(function () {
    "use strict";
    console.log("done");
  })

I get an error similar to the following:

<--- Last few GCs --->

   29305 ms: Scavenge 1398.7 (1457.0) -> 1398.7 (1457.0) MB, 5.0 / 0 ms (+ 1.8 ms in 1 steps since last GC) [allocation failure] [incremental marking delaying mark-sweep].
   30477 ms: Mark-sweep 1398.7 (1457.0) -> 1398.7 (1457.0) MB, 1172.2 / 5 ms (+ 2.7 ms in 2 steps since start of marking, biggest step 1.8 ms) [last resort gc].
   31407 ms: Mark-sweep 1398.7 (1457.0) -> 1398.7 (1457.0) MB, 930.4 / 5 ms [last resort gc].


<--- JS stacktrace --->

==== JS stack trace =========================================

Security context: 0x72d20d36ae1 <JS Object>
    1: /* anonymous */(aka /* anonymous */) [/home/jfremy/prog/nodejs/nodegit/node_modules/nodegit-promise/lib/node-extensions.js:24] [pc=0x28f3e3723a14] (this=0x72d20d04131 <undefined>,resolve=0x1460e70041d1 <JS Function done (SharedFunctionInfo 0x30acef4d48d1)>,reject=0x1460e7004189 <JS Function (SharedFunctionInfo 0x30acef4d4979)>)
    2: tryCallTwo(aka tryCallTwo) [/home/jfremy/prog/nodejs/n...

FATAL ERROR: CALL_AND_RETRY_LAST Allocation failed - process out of memory
Abandon (core dumped)

[johnhaley81]

@implausible this might be related to what you were seeing.

[jfremy]

If I return before retrieving the hunks, the code goes through (does not ensure there is no leak though)

[maxkorp]

How many diffs/patches/hunks are there?
I mean, a leak is possible, but without a memory evaluation of before/after it's hard to say. If it is just memory overload, breaking out your promise chain a bit might help, like so

return commit.getDiff()
  .then(function (diffList) {
    return Promise.all(diffList.map(function (diff) {
      return diff.patches();
    });
  })
  .then(function (patches) {
    return Promise.all(patches.map(function (patch) {
          return patch.hunks();
    })
  })
  .then(function (hunks) {
    return Promise.all(hunks.map(function (hunk) {
      return hunk.lines();
    })
  })
  .then(function () {
      if(commit.parentcount() === 0) {
        return false;
      } else {
        return repo.getCommit(commit.parentId(0));
      }
    });
  });

At least that way, hunks etc can fall out of use and get garbage collected.

[jfremy]

There can be an arbitrary number of patches / hunks. The idea is to be able to traverse the history of a repo and gather stats about it. The example I gave fails on the nodegit repo.
I'll try the idea you mentioned in your code snippet but there'll need to be a few modifications since it would not work in its current state.
The chained promises will at the end return an array of array of array of array. That contains every single commit with every single patch with every single hunk with every single line. Not very scalable...

A stream like API would probably be better. I did not use the one in commit.history since it maintains an array of all the commits visited (so they won't be garbage collected) and does not allow to set a stop condition but I could probably take the code and modify it slightly and check how it goes.

I'll let you know what I find

[jfremy]

So I've been looking more into it.
The alternate way of chaining the promises does not work because it becomes too messy (you get arrays of arrays ...).
I've tried 2 things:

I've slightly updated my code to:

function process(commit) {
  "use strict";
  console.log("commit " + commit.sha());
  return commit.getDiff().then(function (diffList) {
    return Promise.all(diffList.map(function (diff) {
      return diff.patches().then(function (patches) {
        return Promise.all(patches.map(function (patch) {
          return patch.hunks().then(function (hunks) {
            return Promise.all(hunks.map(function (hunk) {
              return hunk.lines().then(function (lines) {
                return true;
              });
            }));
          });
        }));
      })
    }));
  }).then(function () {
    if(commit.parentcount() === 0) {
      commit.free();
      return false;
    } else {
      var nextid = commit.parentId(0);
      commit.free();
      return repo.getCommit(nextid);
    }
  });
}

The only change is the last then which is now outside of the "big loop". So that should mean that it will process one commit at a time. It will gather all the data about the commit and then once this is done, move to the next commit. So no reason for the data not being garbage collected if needed.

That did not work. I still get the same out of memory message as pasted in the first message.

Next I tried to use the event emitter version (from commit.history()).
Here as well, no success, I run into an out of memory situation.
I even removed the commits array from commit.history to make sure the commits could be garbage collected but to no avail.

If I stop just before the hunk.lines().then() part, then it goes through with all methods.

Any suggestion?

[jfremy]

Some further elements. It might not be a memory leak but I think there is still a problem.
The lines for a patch can get pretty large. Looking at the nodegit repo, there is one commit with a hunk that has 785k lines... and guess what, that's where the out of memory happens
I've been able to run my example code on the repo by slightly modifying the part of convenient_hunk.js that create the promise for the lines.

/**
 * The lines in this hunk
 * @async
 * @return {Array<ConvenientLine>}  a promise that resolves to an array of
 *                                      ConvenientLines
 */
function newConvenientLine(line) {
  return new ConvenientLine(line, 0);
}

ConvenientHunk.prototype.lines = function() {
  var _this = this;
  var size = _this.size();
  var linePromises = [];
  var i;

  function makeLinePromise(i) {
    return _this.patch.getLineInHunk(_this.i, i)
      .then(function(line) {
        return new ConvenientLine(line, i);
      });
  }

  for (i = 0; i < size; i++) {
    linePromises.push(_this.patch.getLineInHunk(_this.i, i).then(newConvenientLine));
  }

  return Promise.all(linePromises);
};

the change is that I take the creation of the line promise out of the context of the function, loosing the index along the way instead of using the makeLinePromise function that creates one additional context per line.
With that change, it works.
It's probably just pushing back the time when the OoO will kick out though.

I think that for the lines, there might be need for a different approach than promises. More stream like maybe. Does that make sense?