On Wed, Jun 2, 2021 at 1:18 PM Derrick Stolee <stolee@gmail.com> wrote:

> could you re-run the scenario with GIT_TRACE2_PERF=1 which will
> give the full Git process stack as we reach that rev-list call.

Sorry about the delay, I've been trying to reproduce... reproduceably :)

I now have a whole file of examples and observations, attached (I
assume text attachments are allowed on this mailing list?), which
should be reproducible for anyone as I was able to use the linux
kernel repo to illustrate all cases.

My observations probably use incorrect terminology and/or illustrate a
lack of understanding of the underlying intended behaviors, but here
are the most surprising (not-intentional-seeming) ones:
 * the "rev-list" calls are normal behavior for non-promisor remotes;
they normally complete very fast (with perhaps very little stdin
input??)
 * they happen in "fetch.c", right after the "remote_refs" labelled
code and within the "consume_refs" labelled code
 * for promisor remotes, these rev-list calls are normally
(intentionally?) skipped. They are run only is there is a non-promisor
packfile which contains a commit that is at the "tip" of one of the
(promisor) remote's refs
 * when these rev-list calls happen, they are incredibly, strangely
expensive (a full "git rev-list --all --objects" completes *much*
faster)
 * even weirder, the cost of these calls appears proportional to the
*promisor packfile sizes* - if you specify a more permissive filter at
clone-time, and therefore have more objects actually there (not
missing/skipped) in your promisor packfiles, then it takes up taking
even (much) longer
 * maybe weirdest - all of this goes away when the non-promisor
packfile gets "hidden" behind a more recent promisor packfile;
whatever this check is supposed to do, I don't believe it is
sound/correct.

> This is the critical point: you first cloned without a filter,
> and then converted the remote to a promisor remote without
> marking the pack-files you received from that remote as promisor
> pack-files. That means that Git needs to do some work to discover
> which objects are reachable from promisor packs or not, and that
> extra work is slowing you down.

As noted above, I don't think that work is, in the version I was
testing at least (2.31.1), correct. That said, I may well be
misunderstanding its intent.

> This does make me wonder what happens when Git repacks objects
> created locally and then starts fetching from a promisor remote.

I can confirm that *if* the locally-created non-promisor packfile
contains a commit that is the "tip" of a (promisor) remote branch,
then this will trigger the strange/pathological fetch performance
issue. As soon as you add a "promisor" marker for the packfile, or as
soon as someone else pushes new commits to the branch that then get
fetched as new promisor packfiles in your repo, you're golden, the
fetch process stops doing its "panic - non-promisor packfile found"
behavior... Even though a non-promisor packfile *is still in scope!*
(just not at the tip of a ref for that remote)

Related to this, we have the other notable observation that "git
repack" (without asking to do anything with promisors) ends up doing
the same kind of work as the "rev-list" calls noted above, also
proportional to the size and/or number of objects present in promisor
packfiles; which is a little frustrating when the simplest cure to
*other* performance issue, above, is to make all packfile promissor
ones.

> There are some challenges here, for sure. Most likely also some
> potential gains, but it is unlikely to create a seamless
> experience for what you are trying to do.

There's one other crazy finding I should note explicitly here, which
is that force-pushing a branch can, for reasons I cannot explain,
cause you to redownload all the repo's commits & trees again (during
the forced push, as a just-in-time fetch). And again (if the branch
changes and you force-push again). And again for as long as you're
overwriting others' changes. For the linux kernel repo without blobs,
that's 1.09GB a pop, of purely duplicated promisor packfiles
containing the whole (no-blob-filtered) repo. Interestingly, the
behavior is the same regardless of your configured filter - so the
"blob=none" behavior must be hardcoded or implied somehow in the
codepath that produces this weird outcome.

That said, so far I think everything I'm finding is manageable:
* To avoid "non-promisor packfile" issues in the initial dual-stage
clone, we just need to add extra promisor marker files when we
"upgrade" the remote to a promisor remote.
* Git repack performance will (presumably?) only affect background GC jobs
* Any repack-originated non-promissor packfiles can be cleaned up
(made promissor packfiles) by our tooling
* Any weird re-downloads of the commits+trees during force-pushes
should be OK, they would "only" cost 400MB in our repo

If these are issues that someone is interested in looking into, I'd be
very happy to work with them, but my understanding of the codebase
(and even the language) is... poor.

Thanks,
Tao