Re: [PATCH v4 8/9] midx: read `RIDX` chunk when present

[Taylor Blau <me@ttaylorr.com>]

On Wed, Jan 26, 2022 at 04:10:08PM +0100, Ævar Arnfjörð Bjarmason wrote:
>
> On Tue, Jan 25 2022, Taylor Blau wrote:
>
> > @@ -298,9 +298,26 @@ int load_midx_revindex(struct multi_pack_index *m)
> >  {
> >  	struct strbuf revindex_name = STRBUF_INIT;
> >  	int ret;
> > +
>
> nit: good addition for style in general, but a stay whitespace change in
> an otherwise narrowly focused patch.
>
> >  	if (m->revindex_data)
> >  		return 0;
> >
> > +	if (m->chunk_revindex) {
> > +		/*
> > +		 * If the MIDX `m` has a `RIDX` chunk, then use its contents for
> > +		 * the reverse index instead of trying to load a separate `.rev`
> > +		 * file.
> > +		 *
> > +		 * Note that we do *not* set `m->revindex_map` here, since we do
> > +		 * not want to accidentally call munmap() in the middle of the
> > +		 * MIDX.
> > +		 */
> > +		trace2_data_string("load_midx_revindex", the_repository,
> > +				   "source", "midx");
> > +		m->revindex_data = (const uint32_t *)m->chunk_revindex;
> > +		return 0;
> > +	}
> > +
> >  	trace2_data_string("load_midx_revindex", the_repository,
> >  			   "source", "rev");
>
> This trace2_data_string() is repeated with just "midx" v.s. "rev"
> parameters.
>
> I was going to suggest that maybe we should add a "goto" here, since
> you're trying to juggle the early return and logging this.

Right; that matches how we handle the two cases separately here. I'm not
sure exactly what you're suggesting with by adding a "goto" label; I
think it would make things more awkward rather than more readable, but
it's certainly possible that I'm missing something.

> But wouldn't this be both simpler and give you better logging if it was
> the usual region start/end pattern, with just the "data string"
> in-between?
>
> Then you'd get both performance data on the index loading, and the
> source.
>
> Or maybe it's overkill in this case, I don't know...

I think it's overkill; in the "embedded" case (which we expect to be
pretty common in the future) we're not really measuring anything except
for the pointer assignment.

We *could* instrument pack-revindex.c::load_revindex_from_disk(), but I
think that's a separate issue from this one.

Thanks,
Taylor