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