Re: [PATCH v2 4/4] packed-backend: mmap large "packed-refs" file during fsck

From: Jeff King <peff@peff.net>
Date: May 9, 2025, 15:59 UTC
Message-ID: <20250509155934.GA25686@coredump.intra.peff.net>
In-Reply-To: <aB4dflpFNW4mJlq6@ArchLinux>

On Fri, May 09, 2025 at 11:21:34PM +0800, shejialuo wrote:

Show 10 quoted lines

> > > -	struct strbuf packed_ref_content = STRBUF_INIT;
> > > +	struct snapshot *snapshot = xcalloc(1, sizeof(*snapshot));
> > 
> > Minor, but is there any reason to allocate this here and not just:
> > 
> >   struct snapshot snapshot = { 0 };
> > 
> > ?
> 
> I simply copy the code from the existing code... I will change.

Ah, I see. The existing code must allocate on the heap because it is returning the snapshot to its caller. But here the variable is completely local to the function.

However, if we stop using mmap_strategy altogether and just use xmmap() directly, I don't think you'd even need a snapshot variable.

Show 13 quoted lines

> > Why are we unmapping here before we use the content? That will create an
> > allocated in-memory copy of the mmap'd content. I thought the whole
> > point here was to avoid doing so.
> > 
> 
> I simply follow how "create_snapshot" does. Actually, I am also quite
> confused about this. If we would eventually copy the content into the
> user space's memory. What is the reason that we mmap at Windows in the
> first place?
> 
> My understanding is that after mmaping, we need to do some sanity checks
> and then if there is a need, we may sort the "packed-refs" file. So, we
> would improve some efficiency at Windows for this part?

Ah, yes, that makes sense for the existing code. If we do have to sort, then mapping on Windows means we could sort into our internal buffer, saving an extra copy. That is probably a rare case these days (once upon a time the file was not guaranteed to be sorted, but these days the writer makes sure it is sorted and puts a marker in the header claiming it is so). So maybe that approach is not as useful as it once was, but I don't know if it's worth spending effort to rip it out now.

I don't think any of that applies for packed_fsck(), though, since it is just processing the file linearly in that case (rather than making a sorted copy).

-Peff