diff mbox series

[1/5] merge-tree --stdin: flush stdout to avoid deadlock

Message ID 3b3179785098580f3336bb24bdbaf0aa1366bfcd.1739723830.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series merge-tree --stdin: flush stdout | expand

Commit Message

Phillip Wood Feb. 16, 2025, 4:37 p.m. UTC 
From: Phillip Wood <phillip.wood@dunelm.org.uk>

If a process tries to read the output from "git merge-tree --stdin"
before it closes merge-tree's stdin then it deadlocks. This happens
because merge-tree does not flush its output before trying to read
another line of input and means that it is not possible to cherry-pick a
sequence of commits using "git merge-tree --stdin". Fix this by calling
maybe_flush_or_die() before trying to read the next line of
input. Flushing the output after each merge does not seem to affect the
performance, any difference is lost in the noise even after increasing
the number of runs.

$ git rev-list --merges --parents -n100 origin/master |
	sed 's/^[^ ]* //' >/tmp/merges
$ hyperfine -L flush 0,1 --warmup 1 --runs 30 \
	'GIT_FLUSH={flush} ./git merge-tree --stdin </tmp/merges'
Benchmark 1: GIT_FLUSH=0 ./git merge-tree --stdin </tmp/merges
  Time (mean ± σ):     546.6 ms ±  11.7 ms    [User: 503.2 ms, System: 40.9 ms]
  Range (min … max):   535.9 ms … 567.7 ms    30 runs

Benchmark 2: GIT_FLUSH=1 ./git merge-tree --stdin </tmp/merges
  Time (mean ± σ):     546.9 ms ±  12.0 ms    [User: 505.9 ms, System: 38.9 ms]
  Range (min … max):   529.8 ms … 570.0 ms    30 runs

Summary
  'GIT_FLUSH=0 ./git merge-tree --stdin </tmp/merges' ran
    1.00 ± 0.03 times faster than 'GIT_FLUSH=1 ./git merge-tree --stdin </tmp/merges'

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/merge-tree.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Elijah Newren Feb. 17, 2025, 8:01 p.m. UTC | #1 
On Sun, Feb 16, 2025 at 8:37 AM Phillip Wood via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> If a process tries to read the output from "git merge-tree --stdin"
> before it closes merge-tree's stdin then it deadlocks. This happens
> because merge-tree does not flush its output before trying to read
> another line of input and means that it is not possible to cherry-pick a
> sequence of commits using "git merge-tree --stdin". Fix this by calling
> maybe_flush_or_die() before trying to read the next line of
> input.

Makes sense.

> Flushing the output after each merge does not seem to affect the
> performance, any difference is lost in the noise even after increasing
> the number of runs.
>
> $ git rev-list --merges --parents -n100 origin/master |
>         sed 's/^[^ ]* //' >/tmp/merges
> $ hyperfine -L flush 0,1 --warmup 1 --runs 30 \
>         'GIT_FLUSH={flush} ./git merge-tree --stdin </tmp/merges'
> Benchmark 1: GIT_FLUSH=0 ./git merge-tree --stdin </tmp/merges
>   Time (mean ± σ):     546.6 ms ±  11.7 ms    [User: 503.2 ms, System: 40.9 ms]
>   Range (min … max):   535.9 ms … 567.7 ms    30 runs
>
> Benchmark 2: GIT_FLUSH=1 ./git merge-tree --stdin </tmp/merges
>   Time (mean ± σ):     546.9 ms ±  12.0 ms    [User: 505.9 ms, System: 38.9 ms]
>   Range (min … max):   529.8 ms … 570.0 ms    30 runs
>
> Summary
>   'GIT_FLUSH=0 ./git merge-tree --stdin </tmp/merges' ran
>     1.00 ± 0.03 times faster than 'GIT_FLUSH=1 ./git merge-tree --stdin </tmp/merges'

Nice; thanks for checking and providing these stats.

> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  builtin/merge-tree.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
> index 9a6c8b4e4cf..57f4340faba 100644
> --- a/builtin/merge-tree.c
> +++ b/builtin/merge-tree.c
> @@ -18,6 +18,7 @@
>  #include "tree.h"
>  #include "config.h"
>  #include "strvec.h"
> +#include "write-or-die.h"
>
>  static int line_termination = '\n';
>
> @@ -623,6 +624,7 @@ int cmd_merge_tree(int argc,
>                         } else {
>                                 die(_("malformed input line: '%s'."), buf.buf);
>                         }
> +                       maybe_flush_or_die(stdout, "stdout");
>
>                         if (result < 0)
>                                 die(_("merging cannot continue; got unclean result of %d"), result);
> --
> gitgitgadget

Looks good to me.
diff mbox series

Patch

diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index 9a6c8b4e4cf..57f4340faba 100644
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -18,6 +18,7 @@ 
 #include "tree.h"
 #include "config.h"
 #include "strvec.h"
+#include "write-or-die.h"
 
 static int line_termination = '\n';
 
@@ -623,6 +624,7 @@  int cmd_merge_tree(int argc,
 			} else {
 				die(_("malformed input line: '%s'."), buf.buf);
 			}
+			maybe_flush_or_die(stdout, "stdout");
 
 			if (result < 0)
 				die(_("merging cannot continue; got unclean result of %d"), result);