On Fri, Jul 23, 2021 at 06:00:47AM -0400, Jeff King wrote:
> On Wed, Jul 21, 2021 at 07:01:16PM -0400, Taylor Blau wrote:
> > > > + if (bitmap_is_midx(bitmap_git)) {
> > > > + /*
> > > > + * Can't reuse from a non-preferred pack (see
> > > > + * above).
> > > > + */
> > > > + if (pos + offset >= objects_nr)
> > > > + continue;
> > > > + }
> > > > + try_partial_reuse(bitmap_git, pack, pos + offset, reuse, &w_curs);
> > >
> > > ...and this likewise makes sure we never go past that first pack. Good.
> > >
> > > I think this "continue" could actually be a "break", as the loop is
> > > iterating over "offset" (and "pos + offset" always gets larger). In
> > > fact, it could break out of the outer loop as well (which is
> > > incrementing "pos"). It's probably a pretty small efficiency in
> > > practice, though.
> >
> > Yeah; you're right. And we'll save up to BITS_IN_EWORD cycles of this
> > loop. (I wonder if smart-enough compilers will realize the same
> > optimization that you did and turn that `continue` into a `break`
> > automatically, but that's neither here nor there).
>
> If you break all the way out, then it saves iterating over all of those
> other words that are not in the first pack, too. I.e., if your bitmap
> has 10 million bits (for a 10-million object clone), but your first pack
> only has a million objects in it, we'll call try_partial_reuse() 9
> million extra times.
>
> Fortunately, each call is super cheap, because the first thing it does
> is check if the requested bit is past the end of the pack. Which kind of
> makes me wonder if we could simplify this further by just letting
> try_partial_reuse() tell us when there's no point going further:
>
> [snip suggested diff]
All looks pretty good to me. I think that a goto is a little easier to
read than two identical "if (ret < 0)" checks. And having a comment
makes it clearer to me than the double if statements. So I'm content do
to that instead.
Thanks,
Taylor
