Re: [PATCH v2] midx: traverse the local MIDX first

From: Jeff King <peff@peff.net>
Date: Aug 28, 2020, 21:19 UTC
Message-ID: <20200828211931.GA2190002@coredump.intra.peff.net>
In-Reply-To: <20200828202213.GA24009@nand.local>

On Fri, Aug 28, 2020 at 04:22:13PM -0400, Taylor Blau wrote:

Show 11 quoted lines

> This patch addresses that by:
> 
>   - placing the local MIDX first in the chain when calling
>     'prepare_multi_pack_index_one()', and
> 
>   - introducing a new 'get_local_multi_pack_index()', which explicitly
>     returns the repository-local MIDX, if any.
> 
> Don't impose an additional order on the MIDX's '->next' pointer beyond
> that the first item in the chain must be local if one exists so that we
> avoid a quadratic insertion.

This version looks fine to me.

Thinking on it a bit more, we probably want this "local one is first" behavior even without it fixing this bug. For normal packs, we always prefer local copies over alternates, under the assumption that alternates are likely to be more expensive to access (e.g., shared nfs).

Of course that somewhat goes out the window since we re-order lookups these days based on where we've found previous objects (my mru stuff, but even before that the single "last pack" variable). But it makes sense to at least start with the local ones being given priority.

I don't think we do any mru tricks with the midx list, so it would stay in local-first mode always, which is reasonable (you probably don't have so many midx's that any reordering is worth it anyway).

It does mean we may consult an alternates midx before any local non-midx packs, which _could_ be slower. But since this is all guesses and heuristics anyway, I'd wait until somebody has a concrete case where they can demonstrate a different ordering scheme works better.

-Peff