Re: [PATCH v2 29/33] refs: resolve symbolic refs first

From: Jeff King <peff@peff.net>
Date: May 12, 2016, 08:25 UTC
Message-ID: <20160512082526.GA20817@sigill.intra.peff.net>
In-Reply-To: <20160512074528.GB10922@sigill.intra.peff.net>

On Thu, May 12, 2016 at 03:45:28AM -0400, Jeff King wrote:

Show 8 quoted lines

> So I'd expect us to hit that lock_ref_for_update() for each of the new
> refs. But then we end up in verify_refname_available_dir(), which wants
> to read all of the loose refs again. So we end up with a quadratic
> number of calls to read_ref_full().
> 
> I haven't found the actual bug yet. It may be something as simple as not
> clearing REF_INCOMPLETE from the loose-ref cache when we ought to. But
> that's a wild (optimistic) guess.

Ah, nope, nothing so simple.

It looks like we get in a chain of:

  1. we want to update a ref, so we end up in
     verify_refname_available_dir to make sure we can do so.

  2. that has to load all of the loose refs in refs/tags, which is
     expensive.

  3. we actually update the ref, which calls clear_loose_ref_cache().

And repeat. Each ref update does the expensive step 2, and it gets larger and larger each time.

I understand why we need to verify_refname_available() on the packed refs. But traditionally we would rely on EISDIR or EEXIST to detect conflicts with the loose refs, rather than loading using our own cache. So I guess that's the new thing that is causing a problem.

-Peff