Re: What's cooking in git.git (Nov 2018, #06; Wed, 21)

From: Jeff King <peff@peff.net>
Date: Nov 24, 2018, 12:09 UTC
Message-ID: <20181124120950.GB19257@sigill.intra.peff.net>
In-Reply-To: <87efbd0xix.fsf@evledraar.gmail.com>

On Thu, Nov 22, 2018 at 07:36:54PM +0100, Ævar Arnfjörð Bjarmason wrote:

Show 9 quoted lines

> > Yeah, my intent had been to circle back around to this, but I just
> > hadn't gotten to it. I'm still pondering a config option or similar,
> > though I remain unconvinced that the cases in which you've showed it
> > being slow are actually realistic or worth worrying about
> 
> FWIW those "used to be 2ms are now 20-40ms" pushes on ext4 are
> representative of the actual prod setup I'm mainly targeting. Now, I
> don't run on ext4 this patch helps there, but it seems plausible that it
> matters to someone who's counting on that performance.

Those are actually the more interesting cases, I think. The ones I'm most skeptical of are the multi-second delays due to having 250K+ objects.

I do also think in the long run we should be fixing the "unreachable always become loose" issues.

> Buh yeah, it's certainly obscure. I don't blame you if you don't want to
> hack on it, and not ejecting this out before 2.20 isn't going to break
> anything for me. But do you mind if I make it configurable as part of my
> post-2.20 "disable collisions?"

No, not at all.

Show 11 quoted lines

> >  (and certainly having an obscure config option is not enough to help
> > most people). If we could have it kick in heuristically, that would be
> > better.
> 
> Aside from this specific scenario. I'd really prefer if we avoid having
> heuristic performance optimizations at all costs.
> 
> Database servers tend to do that sort of thing with their query planner,
> and it results in cases where your entire I/O profile changes overnight
> because you're now on the wrong side of some if/else heuristic about
> whather to use some index or not.

Yes, I agree that's a downside. I just think if we make everybody's performance 10% slower, they're not really going to notice and seek out a config option to flip to fix it.

Show 8 quoted lines

> > However, note that the cache-load for finding abbreviations _must_ have
> > the complete list. And has been loading it for some time. So if you run
> > "git-fetch", for example, you've already been running this code for
> > months (and using the cache in more places is now a free speedup).
> 
> This is reminding me that I need to get around to re-submitting my
> core.validateAbbrev series, which addresses this part of the problem:
> https://public-inbox.org/git/20180608224136.20220-21-avarab@gmail.com/

Yeah, I still agree that is a reasonable thing to do.

Show 8 quoted lines

> > At the very least, we'd want this patch on top, too. I also think René's
> > suggestion use access() is worth pursuing (though to some degree is
> > orthogonal to the cache).
> 
> I haven't had time to test that, and wasn't prioritizing it since I
> figured this was post-2.20. My hunch is it doesn't matter much if at all
> on NFS. The roundtrip time is what matters, whether that roundtrip is
> fstat() or access() probably not.

Yes, that line of reasoning makes sense to me (but I think an easy 2-3% speedup on local filesystems may be worth it).

-Peff