Re: Performance issue of 'git branch'

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Jul 23, 2009, 05:36 UTC
Message-ID: <alpine.LFD.2.01.0907222204310.21520@localhost.localdomain>
In-Reply-To: <7vtz146mgr.fsf@alter.siamese.dyndns.org>

On Wed, 22 Jul 2009, Junio C Hamano wrote:

Show 6 quoted lines

> 
> Hmm, we now have to remember what this patch did, if we ever wanted to
> introduce negative refs later (see ef06b91 do_for_each_ref: perform the
> same sanity check for leftovers., 2006-11-18).  Not exactly nice to spread
> the codepaths that need to be updated.  Is the cold cache performance of
> "git branch" to list your local branches that important?

Hmm. I do think that 7.5s is _way_ too long to wait for something as simple as "what branches do I have?".

And yes, it's also an operation that I'd expect to be quite possibly the first one you do when moving to a new repo, so cold-cache is realistic.

And the 'rawref' thing is exactly the same as the 'ref' version, except it doesn't do the null_sha1 check and the 'has_sha1-file()' check.

And since git branch will do something _better_ than the 'has_sha1_file()' check (by virtue of actually looking up the commit), I don't think that part is an issue. So the only issue is the is_null_sha1() thing.

And quite frankly, while the null-sha1 check may make sense, the way the flag is named right now (DO_FOR_EACH_INCLUDE_BROKEN), I think we might be better off re-thinking things later if we ever end up caring. That 'is_null_sha1()' check should possibly be under a separate flag.

That said, while I think my patch was the simplest and most the problem could certainly have been fixed differently.

For example, instead of using 'for_each_ref()' and then splitting them by kind with that "detect kind" loop, it could instead have done two loops, ie

	if (kinds & REF_LOCAL_BRANCH)
		for_each_ref_in("refs/heads/", append_local, &ref_list);
	if (kinds & REF_REMOTE_BRANCH)
		for_each_ref_in("refs/remotes/", append_remote, &ref_list);

and avoided the other refs we aren't interested in _that_ way instead.

But it would be a bigger and involved patch. It gets really messy too (I tried), because when you use 'for_each_ref_in()' it removes the prefix as it goes along, but then the code in builtin-branch.c wants the prefix after all.

		Linus