Re: [PATCH v2 7/7] pack-objects: use mru list when iterating over packs

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> Here's the code to do the cycle-breaking. Aside from the "hacky" bit,
> it's quite simple.  I added a new state enum to object_entry to handle
> the graph traversal. Since it only needs 2 bits, I _assume_ a compiler
> can fit it in with the bitfields above (or at the very least give it its
> own single byte so we just use what would otherwise be struct padding).
> But I didn't check; if it turns out not to be the case we can easily
> emulate it with two bitfields.  The write_object() check abuses the
> "idx.offset" field to keep the same state, but we could convert it to
> use these flags if we care.

> @@ -1516,6 +1577,13 @@ static void get_object_details(void)
>  			entry->no_try_delta = 1;
>  	}
>  
> +	/*
> +	 * This must happen in a second pass, since we rely on the delta
> +	 * information for the whole list being completed.
> +	 */
> +	for (i = 0; i < to_pack.nr_objects; i++)
> +		break_delta_cycles(&to_pack.objects[i]);
> +
>  	free(sorted_by_offset);
>  }

A potential cycle can only come from reusing deltas across packs in
an unstable order, that happens way before we do the find_delta()
thing, so this is a good place to have the new call.  While reading
break_delta_cycles(), I was wondering if what it does is safe under
multi-threading but there is no need to worry.

The recursiveness of break-delta-cycles is not too bad for the same
reason why it is OK to recurse in check_delta_limit(), I would guess?

This is not new with this change, but I am not quite sure what in
the current code prevents us from busting the delta limit for reused
ones, though.

> I think my preference is to clean up the "hacky" bit of this patch, and
> then apply the earlier MRU patch on top of it (which takes my repack
> from 44 minutes to 5 minutes for this particular test set).

Yup, with something like this to break the delta chain _and_ allow
an object to go through the usual deltify machinery, I'd say the MRU
patch is a wonderful thing to have.