git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / Atom feed
From: Derrick Stolee <stolee@gmail.com>
To: "René Scharfe" <l.s.r@web.de>, "Git Mailing List" <git@vger.kernel.org>
Cc: Junio C Hamano <gitster@pobox.com>, Chris Torek <chris.torek@gmail.com>
Subject: Re: [PATCH] midx: use buffered I/O to talk to pack-objects
Date: Tue, 11 Aug 2020 13:14:03 -0400
Message-ID: <bf1fcdb6-f7bb-4235-8403-a36c38180ee7@gmail.com> (raw)
In-Reply-To: <3655b3c8-9b60-daec-c3a2-6e3703ec5b3f@web.de>

On 8/11/2020 12:08 PM, René Scharfe wrote:
> Am 03.08.20 um 14:39 schrieb Derrick Stolee:
>> On 8/2/2020 10:38 AM, René Scharfe wrote:
>>> Like f0bca72dc77 (send-pack: use buffered I/O to talk to pack-objects,
>>> 2016-06-08), significantly reduce the number of system calls and
>>> simplify the code for sending object IDs to pack-objects by using
>>> stdio's buffering and handling errors after the loop.
>>
>> Good find. Thanks for doing this important cleanup.
>>
>> Outside of Chris's other feedback, this looks like an obviously
>> correct transformation.
> 
> I spent a surprising amount of time trying to find a solution that is
> easy to use and allows precise error handling.  But now I get second
> thoughts.  The main selling point of buffering is better performance,
> which is achieved by reducing the number of system calls.  How much
> better actually?
> 
> So I get this in my Git repo clone without this patch:
> 
>   $ strace --summary-only --trace=write git multi-pack-index repack --no-progress
>   % time     seconds  usecs/call     calls    errors syscall
>   ------ ----------- ----------- --------- --------- ----------------
>   100.00    2.237478           2    921650           write
>   ------ ----------- ----------- --------- --------- ----------------
>   100.00    2.237478                921650           total
> 
> And here's the same with the patch:
> 
>   % time     seconds  usecs/call     calls    errors syscall
>   ------ ----------- ----------- --------- --------- ----------------
>   100.00    0.013293           2      4613           write
>   ------ ----------- ----------- --------- --------- ----------------
>   100.00    0.013293                  4613           total
> 
> Awesome, right?  write(2) calls are down by a factor of almost 200 and
> the time spent on them is reduced significantly, as advertised.  Let's
> ask hyperfine for a second opinion though.  Without this patch:
> 
>   Benchmark #1: git multi-pack-index repack --no-progress
>     Time (mean ± σ):      1.652 s ±  0.206 s    [User: 1.383 s, System: 0.317 s]
>     Range (min … max):    1.426 s …  1.890 s    10 runs
> 
> And the same with this patch applied:
> 
>     Time (mean ± σ):      1.635 s ±  0.199 s    [User: 1.363 s, System: 0.204 s]
>     Range (min … max):    1.430 s …  1.871 s    10 runs
> 
> OK, so system time is down by ca. 50%, but the total duration is
> basically unchanged.  It seems strace added quite some overhead to our
> measurement above.
> 
> Anyway, now I wonder if adding our own buffer on top if the
> OS-internal pipe buffer is actually worth it.  The numbers above are
> from Debian testing , by the way.  Perhaps buffering still pays off on
> operating systems with slower pipes..

For what it's worth, I took your patch and applied it on Git for Windows
and tested 'git multi-pack-index repack' on my copy of the Git repo
(which includes Git for Windows and microsoft/git for a total of
1.7 million objects) and saw the time improve from 22.3s to 16.6s!

The "Enumerating objects" progress bar was visibly faster when I was
watching the command.

I was not expecting such a huge speed bump, seeing how the objects
are being repacked, so this command includes complicated processes
like delta compression an zlib compression.

Thanks! This is definitely worth the speed boost on Windows.

-Stolee

  reply	other threads:[~2020-08-11 17:14 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-02 14:38 René Scharfe
2020-08-02 16:11 ` Chris Torek
2020-08-03 18:10   ` Johannes Sixt
2020-08-03 22:27     ` René Scharfe
2020-08-04  4:31       ` René Scharfe
2020-08-04  4:37         ` Junio C Hamano
2020-08-03 12:39 ` Derrick Stolee
2020-08-11 16:08   ` René Scharfe
2020-08-11 17:14     ` Derrick Stolee [this message]
2020-08-12 16:52 ` [PATCH v2] " René Scharfe
2020-08-12 20:28   ` Junio C Hamano
2020-08-12 20:31     ` Junio C Hamano
2020-08-13  9:11       ` Jeff King
2020-08-13  9:06     ` 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=bf1fcdb6-f7bb-4235-8403-a36c38180ee7@gmail.com \
    --to=stolee@gmail.com \
    --cc=chris.torek@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=l.s.r@web.de \
    /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

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for the project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git