Browse Source

Fix StackOverflowError in RevCommit.carryFlags on deep side graphs

Copying flags through a graph with deep side branches can cause
StackOverflowError. The recursive step to visit the 2nd parent of
a merge commit can overflow the stack if these are themselves very
deep histories with many branches.

Rewrite the loop to iterate up to 500 recursive steps deep before
unwinding the stack and running the remaining parts of the graph
using a dynamically allocated FIFORevQueue.

This approach still allows simple graphs that mostly merge short
lived topic branches into master to copy flags with no dynamic
memory allocation, relying only on temporary stack extensions.
Larger more complex graphs only pay the allocation penalities
if copying has to extend outwards "very far" in the graph, which
is unlikely for many coloring based algorithms.

Change-Id: I1882e6832c916e27dd5f6b7602d9caf66fb39c84
stable-3.4
Shawn Pearce 11 years ago
parent
commit
f716ad6d54
  1. 79
      org.eclipse.jgit/src/org/eclipse/jgit/revwalk/RevCommit.java

79
org.eclipse.jgit/src/org/eclipse/jgit/revwalk/RevCommit.java

@ -63,6 +63,8 @@ import org.eclipse.jgit.util.StringUtils;
/** A commit reference to a commit in the DAG. */ /** A commit reference to a commit in the DAG. */
public class RevCommit extends RevObject { public class RevCommit extends RevObject {
private static final int STACK_DEPTH = 500;
/** /**
* Parse a commit from its canonical format. * Parse a commit from its canonical format.
* *
@ -213,27 +215,72 @@ public class RevCommit extends RevObject {
return Constants.OBJ_COMMIT; return Constants.OBJ_COMMIT;
} }
static void carryFlags(RevCommit c, final int carry) { static void carryFlags(RevCommit c, int carry) {
for (;;) { FIFORevQueue q = carryFlags1(c, carry, 0);
final RevCommit[] pList = c.parents; if (q != null)
if (pList == null) slowCarryFlags(q, carry);
return; }
final int n = pList.length;
if (n == 0) private static FIFORevQueue carryFlags1(RevCommit c, int carry, int depth) {
return; for(;;) {
RevCommit[] pList = c.parents;
for (int i = 1; i < n; i++) { if (pList == null || pList.length == 0)
final RevCommit p = pList[i]; return null;
if ((p.flags & carry) == carry) if (pList.length != 1) {
continue; if (depth == STACK_DEPTH)
p.flags |= carry; return defer(c);
carryFlags(p, carry); for (int i = 1; i < pList.length; i++) {
RevCommit p = pList[i];
if ((p.flags & carry) == carry)
continue;
p.flags |= carry;
FIFORevQueue q = carryFlags1(c, carry, depth + 1);
if (q != null)
return defer(q, carry, pList, i + 1);
}
} }
c = pList[0]; c = pList[0];
if ((c.flags & carry) == carry) if ((c.flags & carry) == carry)
return; return null;
c.flags |= carry;
}
}
private static FIFORevQueue defer(RevCommit c) {
FIFORevQueue q = new FIFORevQueue();
q.add(c);
return q;
}
private static FIFORevQueue defer(FIFORevQueue q, int carry,
RevCommit[] pList, int i) {
// In normal case the caller will run pList[0] in a tail recursive
// fashion by updating the variable. However the caller is unwinding
// the stack and will skip that pList[0] execution step.
carryOneStep(q, carry, pList[0]);
// Remaining parents (if any) need to have flags checked and be
// enqueued if they have ancestors.
for (; i < pList.length; i++)
carryOneStep(q, carry, pList[i]);
return q;
}
private static void slowCarryFlags(FIFORevQueue q, int carry) {
// Commits in q have non-null parent arrays and have set all
// flags in carry. This loop finishes copying over the graph.
for (RevCommit c; (c = q.next()) != null;) {
for (RevCommit p : c.parents)
carryOneStep(q, carry, p);
}
}
private static void carryOneStep(FIFORevQueue q, int carry, RevCommit c) {
if ((c.flags & carry) != carry) {
c.flags |= carry; c.flags |= carry;
if (c.parents != null)
q.add(c);
} }
} }

Loading…
Cancel
Save