[PATCH v3] t: add "test-tool genbytes" to avoid slow tr(1) on macOS

["Kristofer Karlsson via GitGitGadget" <gitgitgadget@gmail.com>]

From: Kristofer Karlsson <krka@spotify.com>

Several tests create fixture data consisting of a single repeated
character with "test-tool genzeros <count> | tr '\000' <char>".  This
works well on platforms where tr(1) is implemented efficiently.

The implementation of tr(1) on macOS supports multi-byte characters,
and processes its input one character at a time through
getwchar() and putwchar(), even in the C locale.  For small inputs
that does not matter, but t4205 generates more than 2GB of data this
way, which makes that test much slower on macOS than on Linux: on the
macOS CI runners, tr(1) alone spends 95 to 123 seconds of CPU time on
it, compared to about 6 seconds on Linux.

Fix this by adding "test-tool genbytes <char> [<count>]" next to
"genzeros", sharing the same code, which writes the requested
character directly.  Use it in all tests that piped "genzeros"
through tr(1), not just t4205, so that it is the obvious choice for
new tests.  Callers that want NUL bytes keep using "genzeros".

While at it, parse the count with git_parse_signed(), so that an
invalid count is reported as a usage error instead of being silently
treated as 0.

This makes t4205 much faster on macOS in CI.  The Linux jobs, shown
for comparison, show smaller differences, which may partly be due to
normal run-to-run variation:

  job                before    after   change
  osx-gcc            821.1s   243.3s   -70.4%
  osx-reftable       911.0s   194.0s   -78.7%
  linux-asan-ubsan   226.7s   222.1s    -2.0%
  linux-leaks        320.2s   298.8s    -6.7%

Signed-off-by: Kristofer Karlsson <krka@spotify.com>
---
    t: add "test-tool genbytes" to avoid slow tr(1) on macOS
    
    While looking for slow tests in CI, t4205 stood out on macOS: its
    EXPENSIVE tests take several times longer there than on Linux. A CI
    experiment showed that much of the time was spent in tr(1), which is
    very slow on macOS for the 2GB of data that t4205 generates.
    
    This adds "test-tool genbytes []" and uses it instead of "genzeros |
    tr", which is where the speedup comes from.
    
    Changes since v2:
    
     * "genbytes" takes a literal character again ("genbytes a") instead of
       two hex digits ("genbytes 61"), which Patrick found hard to read at
       the call sites.
    
     * "genzeros" and its callers that want NUL bytes are kept as they are,
       and the patch that converted them to "genbytes 00" is dropped. This
       avoids churn in tests that do not need to change, as discussed with
       Johannes. Keeping "genzeros" also means that "genbytes" does not need
       special handling for NUL bytes.
    
     * test-genzeros.c is no longer renamed, and "genbytes" lives next to
       "genzeros" in the same file. Since "genzeros" stays, the current file
       name is still valid and not worth churning.
    
     * The buffer is static again instead of 256 KiB on the stack, as
       suggested by Junio, to avoid using a large part of the limited stack
       space. There is no risk of concurrent use in a run-once helper.
    
     * The count is parsed with git_parse_signed(), so that an invalid count
       is a usage error instead of being silently treated as 0, as suggested
       by Patrick. This applies to "genzeros" as well.
    
    Changes in v2 (subsequently reverted in v3):
    
     * Squashed the rename of test-genzeros.c into the patch that changes
       it, as suggested by Junio.
    
     * "genbytes" took the byte as two lowercase hexadecimal digits instead
       of a literal character, as suggested by Junio. This also worked for
       NUL bytes, so "genbytes 00" replaced special-casing "NUL".

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2301%2Fspkrka%2Ftest-tool-genbytes-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2301/spkrka/test-tool-genbytes-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/2301

Range-diff vs v2:

 1:  54139a3a97 ! 1:  17e5baacdb t: add "test-tool genbytes" to avoid slow tr(1) on macOS
     @@ Commit message
          macOS CI runners, tr(1) alone spends 95 to 123 seconds of CPU time on
          it, compared to about 6 seconds on Linux.
      
     -    Fix this by adding "test-tool genbytes <hex-byte> [<count>]", which
     -    writes the requested byte, given as two lowercase hexadecimal
     -    digits, directly.  Turn "genzeros" into a thin wrapper around the
     -    same code, and rename the file to match.  Use the new helper in all
     -    tests that piped "genzeros" through tr(1), not just t4205, so that
     -    it is the obvious choice for new tests.
     +    Fix this by adding "test-tool genbytes <char> [<count>]" next to
     +    "genzeros", sharing the same code, which writes the requested
     +    character directly.  Use it in all tests that piped "genzeros"
     +    through tr(1), not just t4205, so that it is the obvious choice for
     +    new tests.  Callers that want NUL bytes keep using "genzeros".
     +
     +    While at it, parse the count with git_parse_signed(), so that an
     +    invalid count is reported as a usage error instead of being silently
     +    treated as 0.
      
          This makes t4205 much faster on macOS in CI.  The Linux jobs, shown
     -    for comparison, only save the few seconds that GNU tr(1) took, which
     -    is within the usual noise:
     +    for comparison, show smaller differences, which may partly be due to
     +    normal run-to-run variation:
      
            job                before    after   change
            osx-gcc            821.1s   243.3s   -70.4%
     @@ Commit message
      
          Signed-off-by: Kristofer Karlsson <krka@spotify.com>
      
     - ## Makefile ##
     -@@ Makefile: TEST_BUILTINS_OBJS += test-env-helper.o
     - TEST_BUILTINS_OBJS += test-example-tap.o
     - TEST_BUILTINS_OBJS += test-find-pack.o
     - TEST_BUILTINS_OBJS += test-fsmonitor-client.o
     -+TEST_BUILTINS_OBJS += test-genbytes.o
     - TEST_BUILTINS_OBJS += test-genrandom.o
     --TEST_BUILTINS_OBJS += test-genzeros.o
     - TEST_BUILTINS_OBJS += test-getcwd.o
     - TEST_BUILTINS_OBJS += test-hash-speed.o
     - TEST_BUILTINS_OBJS += test-hash.o
     -
     - ## t/helper/meson.build ##
     -@@ t/helper/meson.build: test_tool_sources = [
     -   'test-example-tap.c',
     -   'test-find-pack.c',
     -   'test-fsmonitor-client.c',
     -+  'test-genbytes.c',
     -   'test-genrandom.c',
     --  'test-genzeros.c',
     -   'test-getcwd.c',
     -   'test-hash-speed.c',
     -   'test-hash.c',
     -
     - ## t/helper/test-genbytes.c (new) ##
     + ## t/helper/test-genzeros.c ##
      @@
     -+#define DISABLE_SIGN_COMPARE_WARNINGS
     -+
     -+#include "test-tool.h"
     -+#include "git-compat-util.h"
     -+#include "hex-ll.h"
     -+
     + 
     + #include "test-tool.h"
     + #include "git-compat-util.h"
     ++#include "parse.h"
     + 
     +-int cmd__genzeros(int argc, const char **argv)
      +/*
      + * Write "count" copies of "byte" to stdout, or an endless stream of them if
      + * "count" is negative.
      + */
     -+static int generate_bytes(char byte, intmax_t count)
     -+{
     -+	char buf[256 * 1024];
     -+	ssize_t n;
     -+
     ++static int generate_bytes(unsigned char byte, intmax_t count)
     + {
     +-	/* static, so that it is NUL-initialized */
     +-	static const char zeros[256 * 1024];
     +-	intmax_t count;
     ++	static char buf[256 * 1024];
     + 	ssize_t n;
     + 
     +-	if (argc > 2) {
     +-		fprintf(stderr, "usage: %s [<count>]\n", argv[0]);
     +-		return 1;
     +-	}
     +-
     +-	count = argc > 1 ? strtoimax(argv[1], NULL, 0) : -1;
      +	memset(buf, byte, sizeof(buf));
     -+
     + 
     +-	/* Writing out individual NUL bytes is slow... */
      +	/* Writing out individual bytes is slow... */
     -+	while (count < 0)
     + 	while (count < 0)
     +-		if (xwrite(1, zeros, ARRAY_SIZE(zeros)) < 0)
      +		if (xwrite(1, buf, ARRAY_SIZE(buf)) < 0)
     -+			die_errno("write error");
     -+
     -+	while (count > 0) {
     + 			die_errno("write error");
     + 
     + 	while (count > 0) {
     +-		n = xwrite(1, zeros,
     +-			   count < ARRAY_SIZE(zeros)
     +-			   ? count : ARRAY_SIZE(zeros));
      +		n = xwrite(1, buf,
      +			   count < ARRAY_SIZE(buf)
      +			   ? count : ARRAY_SIZE(buf));
     -+
     -+		if (n < 0)
     -+			die_errno("write error");
     -+
     -+		count -= n;
     -+	}
     -+
     -+	return 0;
     -+}
     -+
     -+static intmax_t parse_count(int argc, const char **argv, int pos)
     -+{
     -+	return pos < argc ? strtoimax(argv[pos], NULL, 0) : -1;
     -+}
     + 
     + 		if (n < 0)
     + 			die_errno("write error");
     +@@ t/helper/test-genzeros.c: int cmd__genzeros(int argc, const char **argv)
     + 
     + 	return 0;
     + }
      +
      +int cmd__genbytes(int argc, const char **argv)
      +{
     -+	if (argc < 2 || argc > 3 || strlen(argv[1]) != 2 ||
     -+	    strspn(argv[1], "0123456789abcdef") != 2) {
     -+		fprintf(stderr, "usage: %s <hex-byte> [<count>]\n", argv[0]);
     ++	intmax_t count = -1;
     ++
     ++	if (argc < 2 || argc > 3 || strlen(argv[1]) != 1 ||
     ++	    (argc == 3 && !git_parse_signed(argv[2], &count, INTMAX_MAX))) {
     ++		fprintf(stderr, "usage: %s <char> [<count>]\n", argv[0]);
      +		return 1;
      +	}
      +
     -+	return generate_bytes(hex2chr(argv[1]), parse_count(argc, argv, 2));
     ++	return generate_bytes(argv[1][0], count);
      +}
      +
      +int cmd__genzeros(int argc, const char **argv)
      +{
     -+	if (argc > 2) {
     ++	intmax_t count = -1;
     ++
     ++	if (argc > 2 ||
     ++	    (argc == 2 && !git_parse_signed(argv[1], &count, INTMAX_MAX))) {
      +		fprintf(stderr, "usage: %s [<count>]\n", argv[0]);
      +		return 1;
      +	}
      +
     -+	return generate_bytes('\0', parse_count(argc, argv, 1));
     ++	return generate_bytes('\0', count);
      +}
      
     - ## t/helper/test-genzeros.c (deleted) ##
     -@@
     --#define DISABLE_SIGN_COMPARE_WARNINGS
     --
     --#include "test-tool.h"
     --#include "git-compat-util.h"
     --
     --int cmd__genzeros(int argc, const char **argv)
     --{
     --	/* static, so that it is NUL-initialized */
     --	static const char zeros[256 * 1024];
     --	intmax_t count;
     --	ssize_t n;
     --
     --	if (argc > 2) {
     --		fprintf(stderr, "usage: %s [<count>]\n", argv[0]);
     --		return 1;
     --	}
     --
     --	count = argc > 1 ? strtoimax(argv[1], NULL, 0) : -1;
     --
     --	/* Writing out individual NUL bytes is slow... */
     --	while (count < 0)
     --		if (xwrite(1, zeros, ARRAY_SIZE(zeros)) < 0)
     --			die_errno("write error");
     --
     --	while (count > 0) {
     --		n = xwrite(1, zeros,
     --			   count < ARRAY_SIZE(zeros)
     --			   ? count : ARRAY_SIZE(zeros));
     --
     --		if (n < 0)
     --			die_errno("write error");
     --
     --		count -= n;
     --	}
     --
     --	return 0;
     --}
     -
       ## t/helper/test-tool.c ##
      @@ t/helper/test-tool.c: static struct test_cmd cmds[] = {
       	{ "example-tap", cmd__example_tap },
     @@ t/t0613-reftable-write-options.sh: test_expect_success 'small block size fails w
       		cd repo &&
       		test_commit A &&
      -		test-tool genzeros 500 | tr "\000" "a" >logmsg &&
     -+		test-tool genbytes 61 500 >logmsg &&
     ++		test-tool genbytes a 500 >logmsg &&
       		cat >expect <<-EOF &&
       		fatal: update_ref failed for ref ${SQ}refs/heads/logme${SQ}: reftable: transaction failure: entry too large
       		EOF
     @@ t/t4205-log-pretty-formats.sh: test_expect_success EXPENSIVE,SIZE_T_IS_64BIT 'lo
       
       test_expect_success EXPENSIVE,SIZE_T_IS_64BIT 'set up huge commit' '
      -	test-tool genzeros 2147483649 | tr "\000" "1" >expect &&
     -+	test-tool genbytes 31 2147483649 >expect &&
     ++	test-tool genbytes 1 2147483649 >expect &&
       	huge_commit=$(git commit-tree -F expect HEAD^{tree})
       '
       
     @@ t/t5300-pack-object.sh: test_description='git pack-object'
      -	test-tool genzeros 4096 | tr "\000" "a" >a &&
      -	test-tool genzeros 4096 | tr "\000" "b" >b &&
      -	test-tool genzeros 4096 | tr "\000" "c" >c &&
     -+	test-tool genbytes 61 4096 >a &&
     -+	test-tool genbytes 62 4096 >b &&
     -+	test-tool genbytes 63 4096 >c &&
     ++	test-tool genbytes a 4096 >a &&
     ++	test-tool genbytes b 4096 >b &&
     ++	test-tool genbytes c 4096 >c &&
       	test-tool genrandom "seed a" 2097152 >a_big &&
       	test-tool genrandom "seed b" 2097152 >b_big &&
       	git update-index --add a a_big b b_big c &&
     @@ t/t7006-pager.sh: test_expect_success 'setup trace2' '
       test_expect_success 'setup large log output' '
      -	test-tool genzeros 50000 |
      -	tr "\000" "a" |
     -+	test-tool genbytes 61 50000 |
     ++	test-tool genbytes a 50000 |
       	sed "s/a/this is a long commit message/g" >commit-msg &&
       	git commit --allow-empty -F commit-msg
       '
     @@ t/t9850-shell.sh: test_expect_success 'shell allows interactive command' '
       
       test_expect_success 'shell complains of overlong commands' '
      -	test-tool genzeros | tr "\000" "a" |
     -+	test-tool genbytes 61 |
     ++	test-tool genbytes a |
       	test_must_fail git shell 2>err &&
       	test_grep "too long" err
       '
 2:  f49af0e33f < -:  ---------- t: replace "test-tool genzeros" with "genbytes 00"


 t/helper/test-genzeros.c          | 54 ++++++++++++++++++++++---------
 t/helper/test-tool.c              |  1 +
 t/helper/test-tool.h              |  1 +
 t/t0613-reftable-write-options.sh |  2 +-
 t/t4205-log-pretty-formats.sh     |  2 +-
 t/t5300-pack-object.sh            |  6 ++--
 t/t7006-pager.sh                  |  3 +-
 t/t9850-shell.sh                  |  2 +-
 8 files changed, 48 insertions(+), 23 deletions(-)

diff --git a/t/helper/test-genzeros.c b/t/helper/test-genzeros.c
index b895436a32..e8d83d5012 100644
--- a/t/helper/test-genzeros.c
+++ b/t/helper/test-genzeros.c
@@ -2,30 +2,28 @@
 
 #include "test-tool.h"
 #include "git-compat-util.h"
+#include "parse.h"
 
-int cmd__genzeros(int argc, const char **argv)
+/*
+ * Write "count" copies of "byte" to stdout, or an endless stream of them if
+ * "count" is negative.
+ */
+static int generate_bytes(unsigned char byte, intmax_t count)
 {
-	/* static, so that it is NUL-initialized */
-	static const char zeros[256 * 1024];
-	intmax_t count;
+	static char buf[256 * 1024];
 	ssize_t n;
 
-	if (argc > 2) {
-		fprintf(stderr, "usage: %s [<count>]\n", argv[0]);
-		return 1;
-	}
-
-	count = argc > 1 ? strtoimax(argv[1], NULL, 0) : -1;
+	memset(buf, byte, sizeof(buf));
 
-	/* Writing out individual NUL bytes is slow... */
+	/* Writing out individual bytes is slow... */
 	while (count < 0)
-		if (xwrite(1, zeros, ARRAY_SIZE(zeros)) < 0)
+		if (xwrite(1, buf, ARRAY_SIZE(buf)) < 0)
 			die_errno("write error");
 
 	while (count > 0) {
-		n = xwrite(1, zeros,
-			   count < ARRAY_SIZE(zeros)
-			   ? count : ARRAY_SIZE(zeros));
+		n = xwrite(1, buf,
+			   count < ARRAY_SIZE(buf)
+			   ? count : ARRAY_SIZE(buf));
 
 		if (n < 0)
 			die_errno("write error");
@@ -35,3 +33,29 @@ int cmd__genzeros(int argc, const char **argv)
 
 	return 0;
 }
+
+int cmd__genbytes(int argc, const char **argv)
+{
+	intmax_t count = -1;
+
+	if (argc < 2 || argc > 3 || strlen(argv[1]) != 1 ||
+	    (argc == 3 && !git_parse_signed(argv[2], &count, INTMAX_MAX))) {
+		fprintf(stderr, "usage: %s <char> [<count>]\n", argv[0]);
+		return 1;
+	}
+
+	return generate_bytes(argv[1][0], count);
+}
+
+int cmd__genzeros(int argc, const char **argv)
+{
+	intmax_t count = -1;
+
+	if (argc > 2 ||
+	    (argc == 2 && !git_parse_signed(argv[1], &count, INTMAX_MAX))) {
+		fprintf(stderr, "usage: %s [<count>]\n", argv[0]);
+		return 1;
+	}
+
+	return generate_bytes('\0', count);
+}
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index b71a22b43b..af70e2bec7 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -33,6 +33,7 @@ static struct test_cmd cmds[] = {
 	{ "example-tap", cmd__example_tap },
 	{ "find-pack", cmd__find_pack },
 	{ "fsmonitor-client", cmd__fsmonitor_client },
+	{ "genbytes", cmd__genbytes },
 	{ "genrandom", cmd__genrandom },
 	{ "genzeros", cmd__genzeros },
 	{ "getcwd", cmd__getcwd },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index f2885b33d5..c35e7412af 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -26,6 +26,7 @@ int cmd__env_helper(int argc, const char **argv);
 int cmd__example_tap(int argc, const char **argv);
 int cmd__find_pack(int argc, const char **argv);
 int cmd__fsmonitor_client(int argc, const char **argv);
+int cmd__genbytes(int argc, const char **argv);
 int cmd__genrandom(int argc, const char **argv);
 int cmd__genzeros(int argc, const char **argv);
 int cmd__getcwd(int argc, const char **argv);
diff --git a/t/t0613-reftable-write-options.sh b/t/t0613-reftable-write-options.sh
index 0b27d75fe3..0b74ff6157 100755
--- a/t/t0613-reftable-write-options.sh
+++ b/t/t0613-reftable-write-options.sh
@@ -146,7 +146,7 @@ test_expect_success 'small block size fails with large reflog message' '
 	(
 		cd repo &&
 		test_commit A &&
-		test-tool genzeros 500 | tr "\000" "a" >logmsg &&
+		test-tool genbytes a 500 >logmsg &&
 		cat >expect <<-EOF &&
 		fatal: update_ref failed for ref ${SQ}refs/heads/logme${SQ}: reftable: transaction failure: entry too large
 		EOF
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 01b97c8888..894d88c39b 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -1182,7 +1182,7 @@ test_expect_success EXPENSIVE,SIZE_T_IS_64BIT 'log --pretty with huge commit mes
 '
 
 test_expect_success EXPENSIVE,SIZE_T_IS_64BIT 'set up huge commit' '
-	test-tool genzeros 2147483649 | tr "\000" "1" >expect &&
+	test-tool genbytes 1 2147483649 >expect &&
 	huge_commit=$(git commit-tree -F expect HEAD^{tree})
 '
 
diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh
index e5ce478484..4fb6a22145 100755
--- a/t/t5300-pack-object.sh
+++ b/t/t5300-pack-object.sh
@@ -9,9 +9,9 @@ test_description='git pack-object'
 
 test_expect_success 'setup' '
 	rm -f .git/index* &&
-	test-tool genzeros 4096 | tr "\000" "a" >a &&
-	test-tool genzeros 4096 | tr "\000" "b" >b &&
-	test-tool genzeros 4096 | tr "\000" "c" >c &&
+	test-tool genbytes a 4096 >a &&
+	test-tool genbytes b 4096 >b &&
+	test-tool genbytes c 4096 >c &&
 	test-tool genrandom "seed a" 2097152 >a_big &&
 	test-tool genrandom "seed b" 2097152 >b_big &&
 	git update-index --add a a_big b b_big c &&
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 75ca7a25da..8f1f71bf12 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -662,8 +662,7 @@ test_expect_success 'setup trace2' '
 '
 
 test_expect_success 'setup large log output' '
-	test-tool genzeros 50000 |
-	tr "\000" "a" |
+	test-tool genbytes a 50000 |
 	sed "s/a/this is a long commit message/g" >commit-msg &&
 	git commit --allow-empty -F commit-msg
 '
diff --git a/t/t9850-shell.sh b/t/t9850-shell.sh
index 27cc49bbd1..d56edebda7 100755
--- a/t/t9850-shell.sh
+++ b/t/t9850-shell.sh
@@ -30,7 +30,7 @@ test_expect_success 'shell allows interactive command' '
 '
 
 test_expect_success 'shell complains of overlong commands' '
-	test-tool genzeros | tr "\000" "a" |
+	test-tool genbytes a |
 	test_must_fail git shell 2>err &&
 	test_grep "too long" err
 '

base-commit: fbd2d1e765da4d2931cadc4c62d3ca5d5b590e7e
-- 
gitgitgadget