On Mon, Feb 22, 2021 at 07:31:12PM -0500, Jeff King wrote:
> On Wed, Feb 17, 2021 at 10:14:11PM -0500, Taylor Blau wrote:
>
> > Here is another updated version of mine and Peff's series to add a new 'git
> > repack --geometric' mode which supports repacking a repository into a geometric
> > progression of packs by object count.
>
> Thanks. This version looks pretty good to me. I have a few inline
> comments below. Mostly just observations, but there a couple tiny nits
> that I think may justify one more re-roll.
Thanks for taking a look; I agree that your comments do justify a
re-roll. But I think that one can be done without touching any of the
code (or maybe one line of code), depending on my question below.
Let's see...
> > [snip documentation]
>
> We don't have a real procedure for marking something as "off limits" for
> users. IMHO omitting it from the documentation and putting an explicit
> note in the commit message is probably enough. It would be perhaps
> stronger to mark it explicitly as "do not touch" in the documentation,
> but then we are polluting the documentation. :)
I agree; and the second paragraph in the quoted snippet is the "do not
touch" one. So I think this one is good as-is.
> I also wondered if we could piggy-back on the sorting of packed_git,
> which is already in reverse chronological order. But here our primary
> structure is the string-list, so we lose that order.
>
> I'm not sure if your sort function is going the right way, though. It
> does:
>
> > ++static int pack_mtime_cmp(const void *_a, const void *_b)
> > ++{
> > ++ struct packed_git *a = ((const struct string_list_item*)_a)->util;
> > ++ struct packed_git *b = ((const struct string_list_item*)_b)->util;
> > ++
> > ++ if (a->mtime < b->mtime)
> > ++ return -1;
> > ++ else if (b->mtime < a->mtime)
> > ++ return 1;
> > ++ else
> > ++ return 0;
> > ++}
> > ++
>
> Does that give us the packs in increasing chronological order, but then
> decreasing chronological order within the packs themselves?
I agree we should be sorting and not blindly accepting the order that
the caller gave us, but...
"chronological order within the packs themselves" confuses me. I think
that you mean ordering objects within a pack by their offsets. If so,
then yes: this gives you the oldest pack first (and all of its objects
in their original order), then the second oldest (and all of its
objects) and so on.
Could you clarify a bit how you'd expect to sort the objects in two
packs?
> > 17: b5081c01b5 ! 5: 181c104a03 p5303: measure time to repack with keep
> > @@ Metadata
> > ## Commit message ##
> > p5303: measure time to repack with keep
> >
> > - This is the same as the regular repack test, except that we mark the
> > - single base pack as "kept" and use --assume-kept-packs-closed. The
> > - theory is that this should be faster than the normal repack, because
> > - we'll have fewer objects to traverse and process.
> > + Add two new tests to measure repack performance. Both test split the
>
> s/test split/tests split/, I think.
Good eyes, thanks.
> > + Likewise, the scaling is pretty extreme on --stdin-packs:
> > +
> > + 5303.7: repack with --stdin-packs (1) 0.01(0.01+0.00)
> > + 5303.13: repack with --stdin-packs (50) 3.53(12.07+0.24)
> > + 5303.19: repack with --stdin-packs (1000) 195.83(371.82+8.10)
> >
> > That's because the code paths around handling .keep files are known to
> > scale badly; they look in every single pack file to find each object.
>
> Your "that's because" is a little confusing to me. It certainly applies
> to the repack vs repack-with-kept comparisons for a given number of
> packs. But the scaling on the three --stdin-packs tests is high because
> each subsequent test is being asked to do a lot more work. But they're
> still cheaper than the matching "repack" case with a given number of
> packs. Just not _as_ cheap as they would be if the kept code weren't so
> slow.
>
> Would it make sense to reorder those two paragraphs?
I think so. I did add a tiny parenthetical after my "Likewise, the
scaling is pretty extreme [...]" to say "(but each subsequent test is
also being asked to do more work)".
> > ++Loose objects are implicitly included in this "roll-up", without respect
> > ++to their reachability. This is subject to change in the future. This
> > ++option (implying a drastically different repack mode) is not guarenteed
> > ++to work with all other combinations of option to `git repack`).
>
> Likewise, this is a big improvement. But should it make it clear that
> touching loose objects requires --unpacked? I.e., something like:
>
> When `--unpacked` is specified, loose objects are included in this
> "roll-up" without respect to their reachability...
>
> Also, s/guarenteed/guaranteed/.
Agreed on both, thanks.
Thanks,
Taylor
