From: Jonathan Tan <jonathantanmy@google.com>
To: Jonathan Nieder <jrnieder@gmail.com>
Cc: git@vger.kernel.org, gitster@pobox.com, bmwill@google.com
Subject: Re: [PATCH 1/6] fetch-pack: clear marks before everything_local()
Date: Tue, 5 Jun 2018 17:32:14 -0700 [thread overview]
Message-ID: <20180605173214.c3920e67cbbbd463be33bc70@google.com> (raw)
In-Reply-To: <20180605230821.GC9266@aiede.svl.corp.google.com>
On Tue, 5 Jun 2018 16:08:21 -0700
Jonathan Nieder <jrnieder@gmail.com> wrote:
> Hi,
>
> Jonathan Tan wrote:
>
> > If tag following is required when using a transport that does not
> > support tag following, fetch_pack() will be invoked twice in the same
> > process, necessitating a clearing of the object flags used by
> > fetch_pack() sometime during the second invocation. This is currently
> > done in find_common(), which means that the work done by
> > everything_local() in marking complete remote refs as COMMON_REF is
> > wasted.
> >
> > To avoid this wastage, move this clearing from find_common() to its
> > parent function do_fetch_pack(), right before it calls
> > everything_local().
>
> I had to read this a few times and didn't end up understanding it.
>
> Is the idea that this will speed something up? Can you provide e.g.
> "perf stat" output (or even a new perf test in t/perf) demonstrating
> the improvement? Or is it a cleanup?
Firstly, I don't know of a practical way to demonstrate this, because we
don't have an implementation of a transport that does not support tag
following. If we could demonstrate this, I think we can demonstrating it
by constructing a negotiation scenario in which COMMON_REF would have
been helpful, e.g. the following (untested) scenario:
T C (T=tag, C=commit)
|/
O
We run "git fetch C" and on the second round (when we fetch T), if we
wiped the flags *after* everything_local() (as is currently done),
negotiation would send "have C" and "have O". But if we wiped the flags
*before* everything_local(), then C would have the COMMON_REF flag and
we will see that we only send "have C". So we have more efficient
negotiation.
> As an experiment, I tried applying the '-' part of the change without
> the '+' part to get confidence that tests cover this well. To my
> chagrin, all tests still passed. :/
Yes, because we don't have tests against a transport which doesn't
support tag following.
> In the preimage, we call clear_marks in find_common. This is right
> before we start setting up the revision walk, e.g. by inserting
> revisions for each ref. In the postimage, we call clear_marks in
> do_fetch_pack. This is right before we call everything_local.
>
> I end up feeling that I don't understand the code well, neither before
> nor after the patch. Ideas for making it clearer?
One idea is to first separate everything_local() into its side effect
parts and the "true" check that everything is local. I'll do that and
send it as part of v2 of this patch series.
next prev parent reply other threads:[~2018-06-06 0:32 UTC|newest]
Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-06-04 17:29 [PATCH 0/6] Refactor fetch negotiation into its own API Jonathan Tan
2018-06-04 17:29 ` [PATCH 1/6] fetch-pack: clear marks before everything_local() Jonathan Tan
2018-06-05 23:08 ` Jonathan Nieder
2018-06-06 0:32 ` Jonathan Tan [this message]
2018-06-04 17:29 ` [PATCH 2/6] fetch-pack: truly stop negotiation upon ACK ready Jonathan Tan
2018-06-05 23:16 ` Jonathan Nieder
2018-06-05 23:18 ` Jonathan Nieder
2018-06-06 0:38 ` Jonathan Tan
2018-06-04 17:29 ` [PATCH 3/6] fetch-pack: in protocol v2, enqueue commons first Jonathan Tan
2018-06-05 23:30 ` Jonathan Nieder
2018-06-06 2:10 ` Jonathan Tan
2018-06-04 17:29 ` [PATCH 4/6] fetch-pack: make negotiation-related vars local Jonathan Tan
2018-06-05 23:35 ` Jonathan Nieder
2018-06-06 2:12 ` Jonathan Tan
2018-06-04 17:29 ` [PATCH 5/6] fetch-pack: move common check and marking together Jonathan Tan
2018-06-06 0:01 ` Jonathan Nieder
2018-06-06 2:12 ` Jonathan Tan
2018-06-04 17:29 ` [PATCH 6/6] fetch-pack: introduce negotiator API Jonathan Tan
2018-06-06 0:37 ` Jonathan Nieder
2018-06-06 2:17 ` Jonathan Tan
2018-06-06 20:47 ` [PATCH v2 0/8] Refactor fetch negotiation into its own API Jonathan Tan
2018-06-06 20:47 ` [PATCH v2 1/8] fetch-pack: split up everything_local() Jonathan Tan
2018-06-14 17:26 ` Brandon Williams
2018-06-06 20:47 ` [PATCH v2 2/8] fetch-pack: clear marks before re-marking Jonathan Tan
2018-06-06 20:47 ` [PATCH v2 3/8] fetch-pack: directly end negotiation if ACK ready Jonathan Tan
2018-06-14 17:29 ` Brandon Williams
2018-06-14 17:34 ` Brandon Williams
2018-06-06 20:47 ` [PATCH v2 4/8] fetch-pack: use ref adv. to prune "have" sent Jonathan Tan
2018-06-14 17:32 ` Brandon Williams
2018-06-14 19:52 ` Junio C Hamano
2018-06-06 20:47 ` [PATCH v2 5/8] fetch-pack: make negotiation-related vars local Jonathan Tan
2018-06-14 17:38 ` Brandon Williams
2018-06-14 19:36 ` Junio C Hamano
2018-06-06 20:47 ` [PATCH v2 6/8] fetch-pack: move common check and marking together Jonathan Tan
2018-06-06 20:47 ` [PATCH v2 7/8] fetch-pack: introduce negotiator API Jonathan Tan
2018-06-06 20:47 ` [PATCH v2 8/8] negotiator/default: use better style in comments Jonathan Tan
2018-06-14 17:39 ` Brandon Williams
2018-06-14 22:54 ` [PATCH v3 0/7] Refactor fetch negotiation into its own API Jonathan Tan
2018-06-14 22:54 ` [PATCH v3 1/7] fetch-pack: split up everything_local() Jonathan Tan
2018-06-14 22:54 ` [PATCH v3 2/7] fetch-pack: clear marks before re-marking Jonathan Tan
2018-06-14 22:54 ` [PATCH v3 3/7] fetch-pack: directly end negotiation if ACK ready Jonathan Tan
2018-06-15 16:04 ` Junio C Hamano
2018-06-14 22:54 ` [PATCH v3 4/7] fetch-pack: use ref adv. to prune "have" sent Jonathan Tan
2018-06-14 22:54 ` [PATCH v3 5/7] fetch-pack: make negotiation-related vars local Jonathan Tan
2018-06-14 22:54 ` [PATCH v3 6/7] fetch-pack: move common check and marking together Jonathan Tan
2018-06-14 22:54 ` [PATCH v3 7/7] fetch-pack: introduce negotiator API Jonathan Tan
2018-06-25 18:24 ` [PATCH v3 0/7] Refactor fetch negotiation into its own API Brandon Williams
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180605173214.c3920e67cbbbd463be33bc70@google.com \
--to=jonathantanmy@google.com \
--cc=bmwill@google.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=jrnieder@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).