@matheusbsbs / matheus-bagliano / commits / 42b766d765

pack-objects: convert recursion to iteration in break_delta_chain()

The break_delta_chain() function is recursive over the depth of a given delta chain, which can lead to possibly running out of stack space. Normally delta depth is quite small, but if there _is_ a pathological case, this is where we would find and fix it, so we should be more careful. We can do it without recursion at all, but there's a little bit of cleverness needed to do so. It's easiest to explain by covering the less-clever strategies first. The obvious thing to try is just keeping our own stack on the heap. Whenever we would recurse, push the new entry onto the stack and loop instead. But this gets tricky; when we see an ACTIVE entry, we need to care if we just pushed it (in which case it's a cycle) or if we just popped it (in which case we dealt with its bases, and no we need to clear the ACTIVE flag and compute its depth). You can hack around that in various ways, like keeping a "just pushed" flag, but the logic gets muddled. However, we can observe that we do all of our pushes first, and then all of our pops afterwards. In other words, we can do this in two passes. First dig down to the base, stopping when we see a cycle, and pushing each item onto our stack. Then pop the stack elements, clearing the ACTIVE flag and computing the depth for each. This works, and is reasonably elegant. However, why do we need the stack for the second pass? We can just walk the delta pointers again. There's one complication. Popping the stack went over our list in reverse, so we could compute the depth of each entry by incrementing the depth of its base, which we will have just computed. To go forward in the second pass, we have to compute the total depth on the way down, and then assign it as we go. This patch implements this final strategy, because it not only keeps the memory off the stack, but it eliminates it entirely. Credit for the cleverness in that approach goes to Michael Haggerty; bugs are mine. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King committed Jan 27, 2017 at 17:05 UTC 42b766d765feb2e0867954eb665ff05e3441b547

1 file changed +99 -30

builtin/pack-objects.c

+99 -30

@@ -1581,48 +1581,117 @@ static void drop_reused_delta(struct object_entry *entry)

1581 */

1582 static void break_delta_chains(struct object_entry *entry)

1583 {

1584 - /* If it's not a delta, it can't be part of a cycle. */

1585 - if (!entry->delta) {

1586 - entry->dfs_state = DFS_DONE;

1587 - return;

1588 - }

1584 + /*

1585 + * The actual depth of each object we will write is stored as an int,

1586 + * as it cannot exceed our int "depth" limit. But before we break

1587 + * changes based no that limit, we may potentially go as deep as the

1588 + * number of objects, which is elsewhere bounded to a uint32_t.

1589 + */

1590 + uint32_t total_depth;

1591 + struct object_entry *cur, *next;

1592 +

1593 + for (cur = entry, total_depth = 0;

1594 + cur;

1595 + cur = cur->delta, total_depth++) {

1596 + if (cur->dfs_state == DFS_DONE) {

1597 + /*

1598 + * We've already seen this object and know it isn't

1599 + * part of a cycle. We do need to append its depth

1600 + * to our count.

1601 + */

1602 + total_depth += cur->depth;

1603 + break;

1604 + }

1605

1590 - switch (entry->dfs_state) {

1591 - case DFS_NONE:

1606 /*

1593 - * This is the first time we've seen the object. We mark it as

1594 - * part of the active potential cycle and recurse.

1607 + * We break cycles before looping, so an ACTIVE state (or any

1608 + * other cruft which made its way into the state variable)

1609 + * is a bug.

1610 */

1596 - entry->dfs_state = DFS_ACTIVE;

1597 - break_delta_chains(entry->delta);

1611 + if (cur->dfs_state != DFS_NONE)

1612 + die("BUG: confusing delta dfs state in first pass: %d",

1613 + cur->dfs_state);

1614

1615 /*

1600 - * Once we've recursed, our base (if we still have one) knows

1601 - * its depth, so we can compute ours (and check it against

1602 - * the limit).

1616 + * Now we know this is the first time we've seen the object. If

1617 + * it's not a delta, we're done traversing, but we'll mark it

1618 + * done to save time on future traversals.

1619 */

1604 - if (entry->delta) {

1605 - entry->depth = entry->delta->depth + 1;

1606 - if (entry->depth > depth)

1607 - drop_reused_delta(entry);

1620 + if (!cur->delta) {

1621 + cur->dfs_state = DFS_DONE;

1622 + break;

1623 }

1624

1610 - entry->dfs_state = DFS_DONE;

1611 - break;

1625 + /*

1626 + * Mark ourselves as active and see if the next step causes

1627 + * us to cycle to another active object. It's important to do

1628 + * this _before_ we loop, because it impacts where we make the

1629 + * cut, and thus how our total_depth counter works.

1630 + * E.g., We may see a partial loop like:

1631 + *

1632 + * A -> B -> C -> D -> B

1633 + *

1634 + * Cutting B->C breaks the cycle. But now the depth of A is

1635 + * only 1, and our total_depth counter is at 3. The size of the

1636 + * error is always one less than the size of the cycle we

1637 + * broke. Commits C and D were "lost" from A's chain.

1638 + *

1639 + * If we instead cut D->B, then the depth of A is correct at 3.

1640 + * We keep all commits in the chain that we examined.

1641 + */

1642 + cur->dfs_state = DFS_ACTIVE;

1643 + if (cur->delta->dfs_state == DFS_ACTIVE) {

1644 + drop_reused_delta(cur);

1645 + cur->dfs_state = DFS_DONE;

1646 + break;

1647 + }

1648 + }

1649

1613 - case DFS_DONE:

1614 - /* object already examined, and not part of a cycle */

1615 - break;

1650 + /*

1651 + * And now that we've gone all the way to the bottom of the chain, we

1652 + * need to clear the active flags and set the depth fields as

1653 + * appropriate. Unlike the loop above, which can quit when it drops a

1654 + * delta, we need to keep going to look for more depth cuts. So we need

1655 + * an extra "next" pointer to keep going after we reset cur->delta.

1656 + */

1657 + for (cur = entry; cur; cur = next) {

1658 + next = cur->delta;

1659

1617 - case DFS_ACTIVE:

1660 /*

1619 - * We found a cycle that needs broken. It would be correct to

1620 - * break any link in the chain, but it's convenient to

1621 - * break this one.

1661 + * We should have a chain of zero or more ACTIVE states down to

1662 + * a final DONE. We can quit after the DONE, because either it

1663 + * has no bases, or we've already handled them in a previous

1664 + * call.

1665 */

1623 - drop_reused_delta(entry);

1624 - entry->dfs_state = DFS_DONE;

1625 - break;

1666 + if (cur->dfs_state == DFS_DONE)

1667 + break;

1668 + else if (cur->dfs_state != DFS_ACTIVE)

1669 + die("BUG: confusing delta dfs state in second pass: %d",

1670 + cur->dfs_state);

1671 +

1672 + /*

1673 + * If the total_depth is more than depth, then we need to snip

1674 + * the chain into two or more smaller chains that don't exceed

1675 + * the maximum depth. Most of the resulting chains will contain

1676 + * (depth + 1) entries (i.e., depth deltas plus one base), and

1677 + * the last chain (i.e., the one containing entry) will contain

1678 + * whatever entries are left over, namely

1679 + * (total_depth % (depth + 1)) of them.

1680 + *

1681 + * Since we are iterating towards decreasing depth, we need to

1682 + * decrement total_depth as we go, and we need to write to the

1683 + * entry what its final depth will be after all of the

1684 + * snipping. Since we're snipping into chains of length (depth

1685 + * + 1) entries, the final depth of an entry will be its

1686 + * original depth modulo (depth + 1). Any time we encounter an

1687 + * entry whose final depth is supposed to be zero, we snip it

1688 + * from its delta base, thereby making it so.

1689 + */

1690 + cur->depth = (total_depth--) % (depth + 1);

1691 + if (!cur->depth)

1692 + drop_reused_delta(cur);

1693 +

1694 + cur->dfs_state = DFS_DONE;

1695 }

1696 }

1697