git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH v7 00/40] libify apply and use lib in am, part 2

From
Christian Couder <christian.couder@gmail.com>
Date
Jun 13, 2016, 16:09 UTC
Message-ID
<20160613160942.1806-1-chriscool@tuxfamily.org>

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/

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/40 to 31/40 were in v1, v2 and v6.

They finish libifying the apply functionality that was in builtin/apply.c and move it into apply.{c,h}.

The libified functionality is not yet used in `git am` in those patches, as the patch that does that (previously 33/44 and now 39/40) has been been moved towards the end of the series (see below).

The only other change in those patches is that the patch that was making dup_devnull() non static (previously 31/44) has been removed. It was reverted anyway towards the end of v6.

  - Patches 32/40 to 38/40 were in v2 and v6.

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.

The patch which reverted the patch that made dup_devnull() non static has been removed too, as the patch it reverted has been removed.

The patch that made `git am` use the 'be_silent' new feature (previously 41/44) has been squashed into a following patch (see below).

The patch (previously 43/44) that added --silent to `git apply` has been removed too as already planned in v6. 

Other than that some commit messages have been improved.
  - Patch 39/40 was in v1, v2 and v6.

This patch (which was 33/44 in v6) makes `git am` use the libified functionality. It has been been moved towards the end of the series following many reviewers' suggestion to avoid temporarily using dup_devnull() to silence the libified apply functionality.

The patch that made `git am` use the 'be_silent' new feature (previously 41/44) has been squashed into this patch.

Also a call to clear_apply_state() has been added into this patch to avoid memory leaks.

  - Patch 44/44 was new in v6.

It replaces some calls to error() with calls to error_errno(). One line has been changed to fix a warning.

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

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 (40):
  apply: move 'struct apply_state' to apply.h
  builtin/apply: make apply_patch() return -1 instead of die()ing
  builtin/apply: read_patch_file() return -1 instead of die()ing
  builtin/apply: make find_header() return -1 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 -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                | 4868 ++++++++++++++++++++++++++++++++++++++++++++++++
 apply.h                |  133 ++
 builtin/am.c           |   91 +-
 builtin/apply.c        | 4813 +----------------------------------------------
 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, 5129 insertions(+), 4818 deletions(-)
 create mode 100644 apply.c
 create mode 100644 apply.h
-- 
2.9.0.rc2.411.g3e2ca28
Next: Christian Couder
Message 1 of 46 in “libify apply and use lib in am, part 2”
  1. 00/40 libify apply and use lib in am, part 2Christian Couder, Jun 13, 2016
  2. 05/40 builtin/apply: make parse_chunk() return a negative integer on errorChristian Couder, Jun 13, 2016
  3. 01/40 apply: move 'struct apply_state' to apply.hChristian Couder, Jun 13, 2016
  4. Junio C HamanoJun 13, 2016
  5. Christian CouderJun 14, 2016
  6. 07/40 builtin/apply: make parse_whitespace_option() return -1 instead of die()ingChristian Couder, Jun 13, 2016
  7. 09/40 builtin/apply: move init_apply_state() to apply.cChristian Couder, Jun 13, 2016
  8. 13/40 builtin/apply: make apply_all_patches() return -1 on errorChristian Couder, Jun 13, 2016
  9. 20/40 builtin/apply: make add_conflicted_stages_file() return -1 on errorChristian Couder, Jun 13, 2016
  10. 17/40 builtin/apply: change die_on_unsafe_path() to check_unsafe_path()Christian Couder, Jun 13, 2016
  11. 18/40 builtin/apply: make build_fake_ancestor() return -1 on errorChristian Couder, Jun 13, 2016
  12. 23/40 builtin/apply: make write_out_one_result() return -1 on errorChristian Couder, Jun 13, 2016
  13. 26/40 builtin/apply: make create_one_file() return -1 on errorChristian Couder, Jun 13, 2016
  14. 27/40 builtin/apply: rename option parsing functionsChristian Couder, Jun 13, 2016
  15. 28/40 apply: rename and move opt constants to apply.hChristian Couder, Jun 13, 2016
  16. 31/40 environment: add set_index_file()Christian Couder, Jun 13, 2016
  17. 32/40 write_or_die: use warning() instead of fprintf(stderr, ...)Christian Couder, Jun 13, 2016
  18. 22/40 builtin/apply: make create_file() return -1 on errorChristian Couder, Jun 13, 2016
  19. 24/40 builtin/apply: make write_out_results() return -1 on errorChristian Couder, Jun 13, 2016
  20. 34/40 apply: make 'be_silent' incompatible with 'apply_verbosely'Christian Couder, Jun 13, 2016
  21. 40/40 apply: use error_errno() where possibleChristian Couder, Jun 13, 2016
  22. 33/40 apply: add 'be_silent' variable to 'struct apply_state'Christian Couder, Jun 13, 2016
  23. 39/40 builtin/am: use apply api in run_apply()Christian Couder, Jun 13, 2016
  24. 35/40 apply: don't print on stdout when be_silent is setChristian Couder, Jun 13, 2016
  25. 38/40 apply: change error_routine when be_silent is setChristian Couder, Jun 13, 2016
  26. 25/40 builtin/apply: make try_create_file() return -1 on errorChristian Couder, Jun 13, 2016
  27. 36/40 usage: add set_warn_routine()Christian Couder, Jun 13, 2016
  28. 37/40 usage: add get_error_routine() and get_warn_routine()Christian Couder, Jun 13, 2016
  29. 21/40 builtin/apply: make add_index_file() return -1 on errorChristian Couder, Jun 13, 2016
  30. 30/40 apply: make some parsing functions static againChristian Couder, Jun 13, 2016
  31. 08/40 builtin/apply: make parse_ignorewhitespace_option() return -1 instead of die()ingChristian Couder, Jun 13, 2016
  32. 11/40 builtin/apply: make check_apply_state() return -1 instead of die()ingChristian Couder, Jun 13, 2016
  33. 16/40 builtin/apply: make gitdiff_*() return -1 on errorChristian Couder, Jun 13, 2016
  34. 15/40 builtin/apply: make gitdiff_*() return 1 at end of headerChristian Couder, Jun 13, 2016
  35. 03/40 builtin/apply: read_patch_file() return -1 instead of die()ingChristian Couder, Jun 13, 2016
  36. 10/40 apply: make init_apply_state() return -1 instead of exit()ingChristian Couder, Jun 13, 2016
  37. 14/40 builtin/apply: make parse_traditional_patch() return -1 on errorChristian Couder, Jun 13, 2016
  38. 19/40 builtin/apply: make remove_file() return -1 on errorChristian Couder, Jun 13, 2016
  39. 12/40 builtin/apply: move check_apply_state() to apply.cChristian Couder, Jun 13, 2016
  40. 06/40 builtin/apply: make parse_single_patch() return -1 on errorChristian Couder, Jun 13, 2016
  41. 04/40 builtin/apply: make find_header() return -1 instead of die()ingChristian Couder, Jun 13, 2016
  42. 02/40 builtin/apply: make apply_patch() return -1 instead of die()ingChristian Couder, Jun 13, 2016
  43. Junio C HamanoJun 13, 2016
  44. Christian CouderJun 14, 2016
  45. Junio C HamanoJun 14, 2016
  46. Christian CouderJun 13, 2016

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.