Re: [PATCH v2 1/8] packfile: prepare for the existence of '*.rev' files

[Taylor Blau <me@ttaylorr.com>]

On Fri, Jan 22, 2021 at 05:54:18PM -0500, Jeff King wrote:
> All of which is a really verbose way of saying: you might want to add a
> few words after the comma:
>
>   In real-world usage, Git is often performing many operations in the
>   revindex (i.e., rather than asking about a single object, we'd
>   generally ask about a range of history).
>
> :) But hopefully it shows that including the offsets is not really
> making things better for the cold cache anyway.

Thanks for including a compelling argument in favor of the approach that
I took in this patch.

I added something along the lines of what you suggested to the final
paragraph, so now it concludes nicely instead of ending in a comma. I
briefly considered whether I should add something about how these
operations scale and how the warming efforts are really amortized across
all of the objects, but I decided against it.

I think that this argument is already documented here, and that there's
no way to concisely state it in an already long patch. Interested
readers will easily be able to find our discussion here, which is good.

> >  Documentation/technical/pack-format.txt |  17 ++++
> >  builtin/repack.c                        |   1 +
> >  object-store.h                          |   3 +
> >  pack-revindex.c                         | 112 +++++++++++++++++++++---
> >  pack-revindex.h                         |   7 +-
> >  packfile.c                              |  13 ++-
> >  packfile.h                              |   1 +
> >  tmp-objdir.c                            |   4 +-
> >  8 files changed, 145 insertions(+), 13 deletions(-)
>
> Oh, there's a patch here, too. :)

:-).

> It mostly looks good to me. I agree with Junio that "compute" is a
> better verb than "load" for generating the in-memory revindex.

Yeah, I settled on load_pack_revindex() either calling
"create_pack_revindex_in_memory()" or "load_pack_revindex_from_disk()".

> > +static int load_pack_revindex_from_disk(struct packed_git *p)
> > +{
> > +	char *revindex_name;
> > +	int ret;
> > +	if (open_pack_index(p))
> > +		return -1;
> > +
> > +	revindex_name = pack_revindex_filename(p);
> > +
> > +	ret = load_revindex_from_disk(revindex_name,
> > +				      p->num_objects,
> > +				      &p->revindex_map,
> > +				      &p->revindex_size);
> > +	if (ret)
> > +		goto cleanup;
> > +
> > +	p->revindex_data = (char *)p->revindex_map + 12;
>
> Junio mentioned once spot where we lose constness through a cast. This
> is another. I wonder if revindex_map should just be a "char *" to make
> pointer arithmetic easier without having to cast.
>
> But also...
>
> > +	if (p->revindex)
> > +		return p->revindex[pos].nr;
> > +	else
> > +		return get_be32((char *)p->revindex_data + (pos * sizeof(uint32_t)));
>
> If p->revindex_data were "const uint32_t *", then this line would just
> be:
>
>   return get_be32(p->revindex_data + pos);
>
> Not a huge deal either way since the whole point is to abstract this
> behind a function where it only has to be written once. I don't think
> there is any downside from the compiler's view (and we already use this
> trick for the bitmap name-hash cache).

Honestly, I'm not a huge fan of implicitly scaling pos by
sizeof(*p->revindex_data), but I can understand why it reads more
clearly here. I don't really feel strongly either way, so I'm happy to
change it in favor of your suggestion.

Of course, since RIDX_HEADER_SIZE is in bytes, not uint32_t's (and it
has to be, since it's also used in the RIDX_MIN_SIZE macro, which is
compared against the st_size of stating the .rev file), you have to do
gross stuff like:

  p->revindex_data = (const uint32_t *)((const char *)p->revindex_map + RIDX_HEADER_SIZE);

But I guess the tradeoff is worth it, since the readers are easier to
parse.

> Thinking out loud a bit: a .rev file means we're spending an extra map
> per pack (but not a descriptor, since we close after mmap). And like the
> .idx files (but unlike .pack file maps), we don't keep track of these
> and try to close them when under memory pressure. I think that's
> probably OK in terms of bytes. It may mean running up against operating
> system number-of-mmap limits more quickly when you have a very large
> number of packs, as mentioned in:
>
>   https://lore.kernel.org/git/20200601044511.GA2529317@coredump.intra.peff.net/
>
> But this is probably bumping the number of problematic packs from 30k to
> 20k. Both are sufficiently ridiculous that I don't think it matters in
> practice.

Agreed.

> > diff --git a/tmp-objdir.c b/tmp-objdir.c
> > index 42ed4db5d3..da414df14f 100644
> > --- a/tmp-objdir.c
> > +++ b/tmp-objdir.c
> > @@ -187,7 +187,9 @@ static int pack_copy_priority(const char *name)
> >  		return 2;
> >  	if (ends_with(name, ".idx"))
> >  		return 3;
> > -	return 4;
> > +	if (ends_with(name, ".rev"))
> > +		return 4;
> > +	return 5;
> >  }
>
> Probably not super important, but: should the .idx file still come last
> here? Simultaneous readers won't start using the pack until the .idx
> file is present. We'd probably prefer they see the whole thing
> atomically, than see a .idx missing its .rev (they won't ever produce a
> wrong answer, but they'll generate the in-core revindex on the fly when
> they don't need to).
>
> I guess one could argue that .bitmap files should get similar treatment,
> but we'd not generally see those in the quarantine objdir anyway, so
> nobody ever gave it much thought.

Yeah, you're right (.idx files should come last, and probably an
argument to include .bitmap files here, too, exists. I'll leave the
latter as #leftoverbits).

> -Peff

Thanks,
Taylor