git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Jeff King <peff@peff.net>
Cc: "Patrick Marlier \(pamarlie\)" <pamarlie@cisco.com>,
	Jonathan Tan <jonathantanmy@google.com>,
	"git\@vger.kernel.org" <git@vger.kernel.org>
Subject: Re: [PATCH] send-pack: use OBJECT_INFO_QUICK to check negative objects
Date: Sat, 30 Nov 2019 09:08:32 -0800	[thread overview]
Message-ID: <xmqqr21p2sxb.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <20191127123211.GG22221@sigill.intra.peff.net> (Jeff King's message of "Wed, 27 Nov 2019 07:32:11 -0500")

Jeff King <peff@peff.net> writes:

> I think the patch below would help you.
>
> -- >8 --
> Subject: [PATCH] send-pack: use OBJECT_INFO_QUICK to check negative objects
>
> When pushing, we feed pack-objects a list of both positive and negative
> objects. The positive objects are what we want to send, and the negative
> objects are what the other side told us they have, which we can use to
> limit the size of the push.
>
> Before passing along a negative object, send_pack() will make sure we
> actually have it (since we only know about it because the remote
> mentioned it, not because it's one of our refs). So it's expected that
> some of these objects will be missing on the local side. But looking for
> a missing object is more expensive than one that we have: it triggers
> reprepare_packed_git() to handle a racy repack, plus it has to explore
> every alternate's loose object tree (which can be slow if you have a lot
> of them, or have a high-latency filesystem).
>
> This isn't usually a big problem, since repositories you're pushing to
> don't generally have a large number of refs that are unrelated to what
> the client has. But there's no reason such a setup is wrong, and it
> currently performs poorly.
>
> We can fix this by using OBJECT_INFO_QUICK, which tells the lookup
> code that we expect objects to be missing. Notably, it will not re-scan
> the packs, and it will use the loose cache from 61c7711cfe (sha1-file:
> use loose object cache for quick existence check, 2018-11-12).
>
> The downside is that in the rare case that we race with a local repack,
> we might fail to feed some objects to pack-objects, making the resulting
> push larger. But we'd never produce an invalid or incorrect push, just a
> less optimal one. That seems like a reasonable tradeoff, and we already
> do similar things on the fetch side (e.g., when marking COMPLETE
> commits).
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> Interestingly, upload-pack does not use OBJECT_INFO_QUICK when it's
> getting oids from the other side. But I think it could possibly benefit
> in the same way. Nobody seems to have noticed. Perhaps it simply comes
> up less, as servers would tend to have more objects than their clients?

Makes me wonder how many times we wre bitten by the fact that
INFO_SKIP_FETCH_OBJECT does not imply INFO_QUICK.  Perhaps most of
the users of INFO_SKIP_FETCH_OBJECT wants to use INFO_FOR_PREFETCH?

cf. 31f5256c ("sha1-file: split OBJECT_INFO_FOR_PREFETCH", 2019-05-28)

>  send-pack.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/send-pack.c b/send-pack.c
> index 34c77cbb1a..16d6584439 100644
> --- a/send-pack.c
> +++ b/send-pack.c
> @@ -41,7 +41,9 @@ int option_parse_push_signed(const struct option *opt,
>  static void feed_object(const struct object_id *oid, FILE *fh, int negative)
>  {
>  	if (negative &&
> -	    !has_object_file_with_flags(oid, OBJECT_INFO_SKIP_FETCH_OBJECT))
> +	    !has_object_file_with_flags(oid,
> +					OBJECT_INFO_SKIP_FETCH_OBJECT |
> +					OBJECT_INFO_QUICK))
>  		return;
>  
>  	if (negative)

  parent reply	other threads:[~2019-11-30 17:08 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-19 13:12 Push a ref to a remote with many refs Patrick Marlier (pamarlie)
2019-11-25 16:22 ` Patrick Marlier (pamarlie)
2019-11-27 12:32 ` [PATCH] send-pack: use OBJECT_INFO_QUICK to check negative objects Jeff King
2019-11-29  9:22   ` Patrick Marlier (pamarlie)
2019-11-30 17:08   ` Junio C Hamano [this message]
2019-12-03 23:20     ` Jeff King
2019-12-04 20:53       ` Jonathan Tan
2019-12-04 21:37         ` Junio C Hamano
2019-12-04  3:55   ` Jonathan Nieder
2019-12-04  4:05     ` Jeff King
2019-12-10 16:16       ` Patrick Marlier (pamarlie)
2019-12-10 20:27         ` Jeff King

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=xmqqr21p2sxb.fsf@gitster-ct.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=jonathantanmy@google.com \
    --cc=pamarlie@cisco.com \
    --cc=peff@peff.net \
    /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).