Regression in: [PATCH on sb/more-repo-in-api] revision: use commit graph in get_reference()
- From
SZEDER Gábor <szeder.dev@gmail.com>
- Date
- Jan 25, 2019, 15:33 UTC
- Message-ID
- <20190125153348.GF6702@szeder.dev>
- In-Reply-To
- <20181204224238.50966-1-jonathantanmy@google.com>
On Tue, Dec 04, 2018 at 02:42:38PM -0800, Jonathan Tan wrote:
Show 36 quoted lines
> When fetching into a repository, a connectivity check is first made by > check_exist_and_connected() in builtin/fetch.c that runs: > > git rev-list --objects --stdin --not --all --quiet <(list of objects) > > If the client repository has many refs, this command can be slow, > regardless of the nature of the server repository or what is being > fetched. A profiler reveals that most of the time is spent in > setup_revisions() (approx. 60/63), and of the time spent in > setup_revisions(), most of it is spent in parse_object() (approx. > 49/60). This is because setup_revisions() parses the target of every ref > (from "--all"), and parse_object() reads the buffer of the object. > > Reading the buffer is unnecessary if the repository has a commit graph > and if the ref points to a commit (which is typically the case). This > patch uses the commit graph wherever possible; on my computer, when I > run the above command with a list of 1 object on a many-ref repository, > I get a speedup from 1.8s to 1.0s. > > Another way to accomplish this effect would be to modify parse_object() > to use the commit graph if possible; however, I did not want to change > parse_object()'s current behavior of always checking the object > signature of the returned object. > > Signed-off-by: Jonathan Tan <jonathantanmy@google.com> > --- > This is on sb/more-repo-in-api because I'm using the repo_parse_commit() > function. > > A colleague noticed this issue when handling a mirror clone. > > Looking at the bigger picture, the speed of the connectivity check > during a fetch might be further improved by passing only the negotiation > tips (obtained through --negotiation-tip) instead of "--all". This patch > just handles the low-hanging fruit first. > ---
I stumbled upon a regression that bisects down to this commit ec0c5798ee (revision: use commit graph in get_reference(), 2018-12-04):
$ ~/src/git/bin-wrappers/git version git version 2.19.1.566.gec0c5798ee $ ~/src/git/bin-wrappers/git commit-graph write --reachable Computing commit graph generation numbers: 100% (58994/58994), done. $ ~/src/git/bin-wrappers/git status HEAD detached at origin/pu nothing to commit, working tree clean $ ~/src/git/bin-wrappers/git -c core.commitGraph=false describe --dirty v2.20.1-833-gcb3b9e7ee3 $ ~/src/git/bin-wrappers/git -c core.commitGraph=true describe --dirty v2.20.1-833-gcb3b9e7ee3
It's all good with only '--dirty', but watch this with '--all --dirty':
$ ~/src/git/bin-wrappers/git -c core.commitGraph=false describe --all --dirty remotes/origin/pu $ ~/src/git/bin-wrappers/git -c core.commitGraph=true describe --all --dirty remotes/origin/pu-dirty
IOW if the commit-graph is enabled, then my clean worktree is reported as dirty.
And to add a cherry on top of my confusion:
$ git checkout v2.20.0 Previous HEAD position was cb3b9e7ee3 Merge branch 'jh/trace2' into pu HEAD is now at 5d826e9729 Git 2.20 $ ~/src/git/bin-wrappers/git -c core.commitGraph=true describe --all --dirty tags/v2.20.0
It's clean even with '--all' and commit-graph enabled, but watch this:
$ git branch this-will-screw-it-up $ ~/src/git/bin-wrappers/git -c core.commitGraph=true describe --all --dirty tags/v2.20.0-dirty
Have fun! :)
Show 32 quoted lines
> revision.c | 15 ++++++++++++++-
> 1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/revision.c b/revision.c
> index b5108b75ab..e7da2c57ab 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -212,7 +212,20 @@ static struct object *get_reference(struct rev_info *revs, const char *name,
> {
> struct object *object;
>
> - object = parse_object(revs->repo, oid);
> + /*
> + * If the repository has commit graphs, repo_parse_commit() avoids
> + * reading the object buffer, so use it whenever possible.
> + */
> + if (oid_object_info(revs->repo, oid, NULL) == OBJ_COMMIT) {
> + struct commit *c = lookup_commit(revs->repo, oid);
> + if (!repo_parse_commit(revs->repo, c))
> + object = (struct object *) c;
> + else
> + object = NULL;
> + } else {
> + object = parse_object(revs->repo, oid);
> + }
> +
> if (!object) {
> if (revs->ignore_missing)
> return object;
> --
> 2.19.0.271.gfe8321ec05.dirty
>