Re: [PATCH 0/6] address packed-refs speed regressions

From: René Scharfe <l.s.r@web.de>
Date: Apr 5, 2015, 22:39 UTC
Message-ID: <5521B993.1080202@web.de>
In-Reply-To: <20150405185259.GB13096@peff.net>

Am 05.04.2015 um 20:52 schrieb Jeff King:

Show 7 quoted lines

> On Sun, Apr 05, 2015 at 03:41:39PM +0200, René Scharfe wrote:
>> I wonder if pluggable reference backends could help here.  Storing refs
>> in a database table indexed by refname should simplify things.
>
> ...this. I think that effort might be better spent on a ref storage
> format that's more efficient, simpler (with respect to subtle races and
> such), and could provide other features (e.g., transactional atomicity).

Such as a DBMS? :-) Leaving storage details to SQLite or whatever sounds attractive to me because I'm lazy.

> The big plus side of packed-refs improvements is that they "just work"
> without worrying about compatibility issues. But ref storage is local,
> so I'm not sure how big a deal that is in practice.

Adding a dependency is a big step, admittedly, so native improvements might be a better fit. There's a chance that we'd run into issues already solved by specialized database engines long ago, though.

Show 11 quoted lines

>> Short-term, can we avoid the getc()/strbuf_grow() dance e.g. by mapping
>> the packed refs file?  What numbers do you get with the following patch?
>
> It's about 9% faster than my series + the fgets optimization I posted
> (or about 25% than using getc).  Which is certainly nice, but I was
> really hoping to just make strbuf_getline faster for all callers, rather
> than introducing special code for one call-site.  Certainly we could
> generalize the technique (i.e., a struct with the mmap data), but then I
> feel we are somewhat reinventing stdio. Which is maybe a good thing,
> because stdio has a lot of rough edges (as seen here), but it does feel
> a bit like NIH syndrome.

Forgot to say: I like your changes. But if strbuf_getline can only be made fast enough beyond that by duplicating stdio buffering then I feel it's better to take a different way. E.g. dropping the requirement to handle NUL chars and basing it on fgets as Junio suggested in his reply to patch 3 sounds good.

In any case, the packed refs file seems special enough to receive special treatment. Using mmap would make the most sense if we could also avoid copying lines to a strbuf for parsing, though.

René