git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jonathan Tan <jonathantanmy@google.com>
To: gitster@pobox.com
Cc: jonathantanmy@google.com, git@vger.kernel.org, sluongng@gmail.com
Subject: Re: [PATCH 2/2] pack-objects: prefetch objects to be packed
Date: Wed, 22 Jul 2020 14:30:48 -0700	[thread overview]
Message-ID: <20200722213048.1581991-1-jonathantanmy@google.com> (raw)
In-Reply-To: <xmqqzh7s5sv4.fsf@gitster.c.googlers.com>

> Jonathan Tan <jonathantanmy@google.com> writes:
> 
> > My argument is that typically *no* objects are missing, so we should
> > delay the prefetch as much as possible in the hope that we don't need it
> > at all. I admit that if some objects are missing, I don't know where
> > they will be in the to_pack list.
> 
> OK, if the common case we optimize for is where there is no object
> missing, then of course "prefetch missing ones upfront" will spend
> cycles to ensure all objects we will pack exist before going into
> the main loop, so the "delay as much as possible" optimization makes
> sense.

OK.

> >> In any case, the design choice needs to be justified in the log
> >> message.  I am not sure if the lazy decision to prefetch at the last
> >> moment is really worth the code smell.  Perhaps it is, if there is a
> >> reason to believe that it would save extra work compared to the more
> >> naive "if we have promissor remote, prefetch what is missing", but I
> >> do not think I've heard that reason yet.
> >
> > I still think that there is a reason (the extra existence check), but if
> > we think that the extra existence check is fast enough (compared to the
> > other operations in pack-objects) or that there is a way to avoid
> > calling oid_object_info_extended() twice for the same object (even with
> > moving the prefetch loop to the beginning), then I agree that we don't
> > need the lazy decision. (Or if we want to write the simpler code now and
> > only improve the performance if we need it later, that's fine with me
> > too.)
> 
> Now I finally have, I think, heard some of the reasoning behind the
> design decision made before this patch was written.  That should be
> written in the proposed log message.
> 
> Thanks.

OK - how about:

[start]
When an object to be packed is noticed to be missing, prefetch all
to-be-packed objects in one batch.

Most of the time (typically, when serving a fetch or when pushing),
packs consist only of objects that the repo has. To maintain the
performance in this case, the existing object type read is made to also
serve as the object existence check: if the read fails, then we do the
prefetch.

An alternative design is to loop over all the entries in to_pack,
checking the existence of each object, and prefetching if we notice any
missing. This would result in clearer code, but would incur some
performance degradation in the aforementioned most common case due to
the additional object existence checks.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
[end]

The first paragraph is already in the original, and the rest are
additions.

  reply	other threads:[~2020-07-22 21:30 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-21  0:21 [PATCH 0/2] Prefetch objects in pack-objects Jonathan Tan
2020-07-21  0:21 ` [PATCH 1/2] pack-objects: refactor to oid_object_info_extended Jonathan Tan
2020-07-21  0:21 ` [PATCH 2/2] pack-objects: prefetch objects to be packed Jonathan Tan
2020-07-21  1:00   ` Junio C Hamano
2020-07-21 16:37     ` Jonathan Tan
2020-07-21 19:23       ` Junio C Hamano
2020-07-21 21:27         ` Junio C Hamano
2020-07-21 23:37           ` Jonathan Tan
2020-07-21 23:56             ` Junio C Hamano
2020-07-21 23:20         ` Jonathan Tan
2020-07-21 23:51           ` Junio C Hamano
2020-07-22 21:30             ` Jonathan Tan [this message]
2020-07-22 21:45               ` Junio C Hamano

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=20200722213048.1581991-1-jonathantanmy@google.com \
    --to=jonathantanmy@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=sluongng@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).