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
