From: Christian Couder <christian.couder@gmail.com>
To: git@vger.kernel.org
Cc: "Junio C Hamano" <gitster@pobox.com>, "Jeff King" <peff@peff.net>,
"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
"Karsten Blees" <karsten.blees@gmail.com>,
"Nguyen Thai Ngoc Duy" <pclouds@gmail.com>,
"Stefan Beller" <sbeller@google.com>,
"Eric Sunshine" <sunshine@sunshineco.com>,
"Ramsay Jones" <ramsay@ramsayjones.plus.com>,
"Johannes Sixt" <j6t@kdbg.org>, "René Scharfe" <l.s.r@web.de>,
"Christian Couder" <chriscool@tuxfamily.org>
Subject: [PATCH v8 00/41] libify apply and use lib in am, part 2
Date: Mon, 27 Jun 2016 20:23:48 +0200 [thread overview]
Message-ID: <20160627182429.31550-1-chriscool@tuxfamily.org> (raw)
Goal
~~~~
This is a patch series about libifying `git apply` functionality, and
using this libified functionality in `git am`, so that no 'git apply'
process is spawn anymore. This makes `git am` significantly faster, so
`git rebase`, when it uses the am backend, is also significantly
faster.
Previous discussions and patches series
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
This has initially been discussed in the following thread:
http://thread.gmane.org/gmane.comp.version-control.git/287236/
Then the following patch series were sent:
RFC: http://thread.gmane.org/gmane.comp.version-control.git/288489/
v1: http://thread.gmane.org/gmane.comp.version-control.git/292324/
v2: http://thread.gmane.org/gmane.comp.version-control.git/294248/
v3: http://thread.gmane.org/gmane.comp.version-control.git/295429/
v4: http://thread.gmane.org/gmane.comp.version-control.git/296350/
v5: http://thread.gmane.org/gmane.comp.version-control.git/296490/
v6: http://thread.gmane.org/gmane.comp.version-control.git/297024/
v7: http://thread.gmane.org/gmane.comp.version-control.git/297193/
Highlevel view of the patches in the series
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
This new patch series is built on top of the above previous work.
More precisely, this is "part 2" of the full patch series which is
built on top of the "part 1" of the full patch series. And as the
"part 1" is now in "next", this "part 2" is built on top of "next".
- Patches 01/41 is new in v8.
It renames some structs and constants that will be moved into apply.h
to give them a more specific name as suggested by Junio.
- Patches 02/41 to 32/41 were in v1, v2, v6 and v7.
They finish libifying the apply functionality that was in
builtin/apply.c and move it into apply.{c,h}, but the libified
functionality is not yet used in `git am`.
There are a number of changes in these patches compared to v7, so that
apply_all_patches() will return 1 in case some patches don't apply and
128 in case a previously fatal error happened. This makes it possible
for `git apply` to return the same exit code as it used to return
before libification.
To do that a number of functions like find_header(), parse_chunk(),
apply_patch(), etc have been made to return -128 instead of -1 when it
was necessary.
- Patches 33/41 to 39/41 were in v2, v6 and v7.
They implement a way to make the libified apply silent by adding a new
'be_silent' flag into 'struct apply_state'. It is a new feature in the
libified apply functionality.
This could be in a separate series, but unfortunately using the
libified apply in "git am" unmasks the fact that "git am", since it
was a shell script, has been silencing the apply functionality by
redirecting file descriptors to /dev/null and it looks like this is
not acceptable in C.
There are no changes in these patches compared to v7.
- Patch 40/41 was in v1, v2, v6 and v7.
This patch makes `git am` use the libified functionality. It is the
same as in v7.
- Patch 41/41 was in v6 and v7.
It replaces some calls to error() with calls to error_errno(). It is
the nearly the same as in v7. The only change is that one call to
error() is now replaced by error_errno() earlier in the series.
General comments
~~~~~~~~~~~~~~~~
Sorry if this patch series is still long. I can split it into two or
more series if it is prefered.
I will send a diff between this version and the previous one, as a
reply to this email.
The benefits are not just related to not creating new processes. When
`git am` launched a `git apply` process, this new process had to read
the index from disk. Then after the `git apply`process had terminated,
`git am` dropped its index and read the index from disk to get the
index that had been modified by the `git apply`process. This was
inefficient and also prevented the split-index mechanism to provide
many performance benefits.
Using this series as rebase material, Duy explains it like this:
> Without the series, the picture is not so surprising. We run git-apply
> 80+ times, each consists of this sequence
>
> read index
> write index (cache tree updates only)
> read index again
> optionally initialize name hash (when new entries are added, I guess)
> read packed-refs
> write index
>
> With this series, we run a single git-apply which does
>
> read index (and sharedindex too if in split-index mode)
> initialize name hash
> write index 80+ times
(See: http://thread.gmane.org/gmane.comp.version-control.git/292324/focus=292460)
Links
~~~~~
This patch series is available here:
https://github.com/chriscool/git/commits/libify-apply-use-in-am
The previous versions are available there:
v1: https://github.com/chriscool/git/commits/libify-apply-use-in-am25
v2: https://github.com/chriscool/git/commits/libify-apply-use-in-am54
v6: https://github.com/chriscool/git/commits/libify-apply-use-in-am65
v7: https://github.com/chriscool/git/commits/libify-apply-use-in-am75
Performance numbers
~~~~~~~~~~~~~~~~~~~
Only tests on Linux have been performed using a very early version of
this series. It could be interesting to test on other platforms
especially Windows and perhaps OSX too.
- Around mid April Ævar did a huge many-hundred commit rebase on the
kernel with untracked cache.
command: git rebase --onto 1993b17 52bef0c 29dde7c
Vanilla "next" without split index: 1m54.953s
Vanilla "next" with split index: 1m22.476s
This series on top of "next" without split index: 1m12.034s
This series on top of "next" with split index: 0m15.678s
Ævar used his Debian laptop with SSD.
- Around mid April I tested rebasing 13 commits in Booking.com's
monorepo on a Red Hat 6.5 server with split-index and
GIT_TRACE_PERFORMANCE=1.
With Git v2.8.0, the rebase took 6.375888383 s, with the git am
command launched by the rebase command taking 3.705677431 s.
With this series on top of next, the rebase took 3.044529494 s, with
the git am command launched by the rebase command taking 0.583521168
s.
Christian Couder (41):
apply: make some names more specific
apply: move 'struct apply_state' to apply.h
builtin/apply: make apply_patch() return -1 or -128 instead of
die()ing
builtin/apply: read_patch_file() return -1 instead of die()ing
builtin/apply: make find_header() return -128 instead of die()ing
builtin/apply: make parse_chunk() return a negative integer on error
builtin/apply: make parse_single_patch() return -1 on error
builtin/apply: make parse_whitespace_option() return -1 instead of
die()ing
builtin/apply: make parse_ignorewhitespace_option() return -1 instead
of die()ing
builtin/apply: move init_apply_state() to apply.c
apply: make init_apply_state() return -1 instead of exit()ing
builtin/apply: make check_apply_state() return -1 instead of die()ing
builtin/apply: move check_apply_state() to apply.c
builtin/apply: make apply_all_patches() return 128 or 1 on error
builtin/apply: make parse_traditional_patch() return -1 on error
builtin/apply: make gitdiff_*() return 1 at end of header
builtin/apply: make gitdiff_*() return -1 on error
builtin/apply: change die_on_unsafe_path() to check_unsafe_path()
builtin/apply: make build_fake_ancestor() return -1 on error
builtin/apply: make remove_file() return -1 on error
builtin/apply: make add_conflicted_stages_file() return -1 on error
builtin/apply: make add_index_file() return -1 on error
builtin/apply: make create_file() return -1 on error
builtin/apply: make write_out_one_result() return -1 on error
builtin/apply: make write_out_results() return -1 on error
builtin/apply: make try_create_file() return -1 on error
builtin/apply: make create_one_file() return -1 on error
builtin/apply: rename option parsing functions
apply: rename and move opt constants to apply.h
Move libified code from builtin/apply.c to apply.{c,h}
apply: make some parsing functions static again
environment: add set_index_file()
write_or_die: use warning() instead of fprintf(stderr, ...)
apply: add 'be_silent' variable to 'struct apply_state'
apply: make 'be_silent' incompatible with 'apply_verbosely'
apply: don't print on stdout when be_silent is set
usage: add set_warn_routine()
usage: add get_error_routine() and get_warn_routine()
apply: change error_routine when be_silent is set
builtin/am: use apply api in run_apply()
apply: use error_errno() where possible
Makefile | 1 +
apply.c | 4891 ++++++++++++++++++++++++++++++++++++++++++++++++
apply.h | 133 ++
builtin/am.c | 91 +-
builtin/apply.c | 4803 +----------------------------------------------
cache.h | 1 +
environment.c | 10 +
git-compat-util.h | 3 +
t/t4012-diff-binary.sh | 4 +-
t/t4254-am-corrupt.sh | 2 +-
usage.c | 15 +
write_or_die.c | 6 +-
12 files changed, 5147 insertions(+), 4813 deletions(-)
create mode 100644 apply.c
create mode 100644 apply.h
--
2.9.0.172.gfb57a78
next reply other threads:[~2016-06-27 18:24 UTC|newest]
Thread overview: 59+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-06-27 18:23 Christian Couder [this message]
2016-06-27 18:23 ` [PATCH v8 01/41] apply: make some names more specific Christian Couder
2016-06-27 18:23 ` [PATCH v8 02/41] apply: move 'struct apply_state' to apply.h Christian Couder
2016-06-27 18:23 ` [PATCH v8 03/41] builtin/apply: make apply_patch() return -1 or -128 instead of die()ing Christian Couder
2016-06-27 18:23 ` [PATCH v8 04/41] builtin/apply: read_patch_file() return -1 " Christian Couder
2016-06-27 18:23 ` [PATCH v8 05/41] builtin/apply: make find_header() return -128 " Christian Couder
2016-06-27 18:23 ` [PATCH v8 06/41] builtin/apply: make parse_chunk() return a negative integer on error Christian Couder
2016-06-27 18:23 ` [PATCH v8 07/41] builtin/apply: make parse_single_patch() return -1 " Christian Couder
2016-06-27 18:23 ` [PATCH v8 08/41] builtin/apply: make parse_whitespace_option() return -1 instead of die()ing Christian Couder
2016-06-27 18:23 ` [PATCH v8 09/41] builtin/apply: make parse_ignorewhitespace_option() " Christian Couder
2016-06-27 18:23 ` [PATCH v8 10/41] builtin/apply: move init_apply_state() to apply.c Christian Couder
2016-06-27 18:23 ` [PATCH v8 11/41] apply: make init_apply_state() return -1 instead of exit()ing Christian Couder
2016-06-27 18:24 ` [PATCH v8 12/41] builtin/apply: make check_apply_state() return -1 instead of die()ing Christian Couder
2016-06-27 18:24 ` [PATCH v8 13/41] builtin/apply: move check_apply_state() to apply.c Christian Couder
2016-06-27 18:24 ` [PATCH v8 14/41] builtin/apply: make apply_all_patches() return 128 or 1 on error Christian Couder
2016-06-27 18:24 ` [PATCH v8 15/41] builtin/apply: make parse_traditional_patch() return -1 " Christian Couder
2016-06-27 18:24 ` [PATCH v8 16/41] builtin/apply: make gitdiff_*() return 1 at end of header Christian Couder
2016-06-27 18:24 ` [PATCH v8 17/41] builtin/apply: make gitdiff_*() return -1 on error Christian Couder
2016-06-27 18:24 ` [PATCH v8 18/41] builtin/apply: change die_on_unsafe_path() to check_unsafe_path() Christian Couder
2016-06-27 18:24 ` [PATCH v8 19/41] builtin/apply: make build_fake_ancestor() return -1 on error Christian Couder
2016-06-27 18:24 ` [PATCH v8 20/41] builtin/apply: make remove_file() " Christian Couder
2016-06-27 18:24 ` [PATCH v8 21/41] builtin/apply: make add_conflicted_stages_file() " Christian Couder
2016-06-27 18:24 ` [PATCH v8 22/41] builtin/apply: make add_index_file() " Christian Couder
2016-06-27 18:24 ` [PATCH v8 23/41] builtin/apply: make create_file() " Christian Couder
2016-06-27 18:24 ` [PATCH v8 24/41] builtin/apply: make write_out_one_result() " Christian Couder
2016-06-27 18:24 ` [PATCH v8 25/41] builtin/apply: make write_out_results() " Christian Couder
2016-06-27 18:24 ` [PATCH v8 26/41] builtin/apply: make try_create_file() " Christian Couder
2016-06-27 18:24 ` [PATCH v8 27/41] builtin/apply: make create_one_file() " Christian Couder
2016-06-27 18:24 ` [PATCH v8 28/41] builtin/apply: rename option parsing functions Christian Couder
2016-06-27 18:24 ` [PATCH v8 29/41] apply: rename and move opt constants to apply.h Christian Couder
2016-06-27 18:24 ` [PATCH v8 31/41] apply: make some parsing functions static again Christian Couder
2016-06-27 18:24 ` [PATCH v8 32/41] environment: add set_index_file() Christian Couder
2016-07-26 19:28 ` Junio C Hamano
2016-07-27 15:14 ` Duy Nguyen
2016-07-29 14:21 ` Christian Couder
2016-07-29 15:34 ` Duy Nguyen
2016-07-29 18:23 ` Christian Couder
2016-07-29 18:35 ` Duy Nguyen
2016-07-29 18:58 ` Junio C Hamano
2016-06-27 18:24 ` [PATCH v8 33/41] write_or_die: use warning() instead of fprintf(stderr, ...) Christian Couder
2016-06-28 21:39 ` Junio C Hamano
2016-06-30 9:50 ` Christian Couder
2016-06-27 18:24 ` [PATCH v8 34/41] apply: add 'be_silent' variable to 'struct apply_state' Christian Couder
2016-07-26 19:34 ` Junio C Hamano
2016-06-27 18:24 ` [PATCH v8 35/41] apply: make 'be_silent' incompatible with 'apply_verbosely' Christian Couder
2016-07-26 19:37 ` Junio C Hamano
2016-06-27 18:24 ` [PATCH v8 36/41] apply: don't print on stdout when be_silent is set Christian Couder
2016-07-26 19:41 ` Junio C Hamano
2016-06-27 18:24 ` [PATCH v8 37/41] usage: add set_warn_routine() Christian Couder
2016-06-27 18:24 ` [PATCH v8 38/41] usage: add get_error_routine() and get_warn_routine() Christian Couder
2016-06-27 18:24 ` [PATCH v8 39/41] apply: change error_routine when be_silent is set Christian Couder
2016-06-27 18:24 ` [PATCH v8 40/41] builtin/am: use apply api in run_apply() Christian Couder
2016-07-26 19:48 ` Junio C Hamano
2016-06-27 18:24 ` [PATCH v8 41/41] apply: use error_errno() where possible Christian Couder
2016-06-27 18:33 ` [PATCH v8 00/41] libify apply and use lib in am, part 2 Christian Couder
2016-07-26 21:18 ` Junio C Hamano
2016-07-27 6:15 ` Christian Couder
2016-07-27 16:24 ` Junio C Hamano
2016-07-30 17:39 ` Christian Couder
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=20160627182429.31550-1-chriscool@tuxfamily.org \
--to=christian.couder@gmail.com \
--cc=avarab@gmail.com \
--cc=chriscool@tuxfamily.org \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=j6t@kdbg.org \
--cc=karsten.blees@gmail.com \
--cc=l.s.r@web.de \
--cc=pclouds@gmail.com \
--cc=peff@peff.net \
--cc=ramsay@ramsayjones.plus.com \
--cc=sbeller@google.com \
--cc=sunshine@sunshineco.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).