On Thu, Nov 22, 2018 at 07:36:54PM +0100, Ævar Arnfjörð Bjarmason wrote:
> > 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. > > (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. > > 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-ava...@gmail.com/ Yeah, I still agree that is a reasonable thing to do. > > 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
