[PATCH 0/4] test-tool: split up "read-cache" tool

[PATCH 0/4] test-tool: split up "read-cache" tool

[Ævar Arnfjörð Bjarmason]

When the sparse index topic was being discussed I suggested that the
t/helper/read-cache.c tool was getting to the point of doing too many
things and should be split up.

Since that series has landed on master here's that suggestion again in
the form of patches on top of master. The 4/4 patch is a "while I was
at it" addition of an extra perf test for index refreshing.

1. https://lore.kernel.org/git/20210317132814.30175-6-avarab@gmail.com/

Ævar Arnfjörð Bjarmason (4):
  test-tool: split up test-tool read-cache
  test-tools: migrate read-cache-perf to parse_options()
  test-tools: migrate read-cache-again to parse_options()
  read-cache perf: add a perf test for refresh_index()

 Makefile                         |  2 ++
 t/helper/test-read-cache-again.c | 47 +++++++++++++++++++++++++++
 t/helper/test-read-cache-perf.c  | 47 +++++++++++++++++++++++++++
 t/helper/test-read-cache.c       | 56 +++++++++++++-------------------
 t/helper/test-tool.c             |  2 ++
 t/helper/test-tool.h             |  2 ++
 t/perf/p0002-read-cache.sh       |  7 +++-
 t/t7519-status-fsmonitor.sh      |  2 +-
 8 files changed, 130 insertions(+), 35 deletions(-)
 create mode 100644 t/helper/test-read-cache-again.c
 create mode 100644 t/helper/test-read-cache-perf.c

-- 
2.32.0.rc3.434.gd8aed1f08a7

[PATCH 1/4] test-tool: split up test-tool read-cache

[Ævar Arnfjörð Bjarmason]

Since the "test-tool read-cache" was originally added back in
1ecb5ff141 (read-cache: add simple performance test, 2013-06-09) it's
been growing all sorts of bells and whistles that aren't very
conducive to performance testing the index, e.g. it learned how to
read config.

Then in recent changes in e2df6c3972 (test-read-cache: print cache
entries with --table, 2021-03-30) and 2782db3eed (test-tool: don't
force full index, 2021-03-30) we gained even more logic to deal with
sparse index testing.

I think that having one test tool do so many different things makes it
harder to read its code. Let's instead split up the "again" and "perf"
uses for it into their own tools.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile                         |  2 ++
 t/helper/test-read-cache-again.c | 31 ++++++++++++++++++
 t/helper/test-read-cache-perf.c  | 21 ++++++++++++
 t/helper/test-read-cache.c       | 56 +++++++++++++-------------------
 t/helper/test-tool.c             |  2 ++
 t/helper/test-tool.h             |  2 ++
 t/perf/p0002-read-cache.sh       |  2 +-
 t/t7519-status-fsmonitor.sh      |  2 +-
 8 files changed, 83 insertions(+), 35 deletions(-)
 create mode 100644 t/helper/test-read-cache-again.c
 create mode 100644 t/helper/test-read-cache-perf.c

diff --git a/Makefile b/Makefile
index c3565fc0f8..142303bd17 100644
--- a/Makefile
+++ b/Makefile
@@ -732,6 +732,8 @@ TEST_BUILTINS_OBJS += test-prio-queue.o
 TEST_BUILTINS_OBJS += test-proc-receive.o
 TEST_BUILTINS_OBJS += test-progress.o
 TEST_BUILTINS_OBJS += test-reach.o
+TEST_BUILTINS_OBJS += test-read-cache-again.o
+TEST_BUILTINS_OBJS += test-read-cache-perf.o
 TEST_BUILTINS_OBJS += test-read-cache.o
 TEST_BUILTINS_OBJS += test-read-graph.o
 TEST_BUILTINS_OBJS += test-read-midx.o
diff --git a/t/helper/test-read-cache-again.c b/t/helper/test-read-cache-again.c
new file mode 100644
index 0000000000..707db036cb
--- /dev/null
+++ b/t/helper/test-read-cache-again.c
@@ -0,0 +1,31 @@
+#include "test-tool.h"
+#include "cache.h"
+
+int cmd__read_cache_again(int argc, const char **argv)
+{
+	struct repository *r = the_repository;
+	int i, cnt;
+	const char *name;
+
+	if (argc != 2)
+		die("usage: test-tool read-cache-again <count> <file>");
+
+	cnt = strtol(argv[0], NULL, 0);
+	name = argv[2];
+
+	setup_git_directory();
+	for (i = 0; i < cnt; i++) {
+		int pos;
+		repo_read_index(r);
+		refresh_index(r->index, REFRESH_QUIET,
+			      NULL, NULL, NULL);
+		pos = index_name_pos(r->index, name, strlen(name));
+		if (pos < 0)
+			die("%s not in index", name);
+		printf("%s is%s up to date\n", name,
+		       ce_uptodate(r->index->cache[pos]) ? "" : " not");
+		write_file(name, "%d\n", cnt);
+		discard_index(r->index);
+	}
+	return 0;
+}
diff --git a/t/helper/test-read-cache-perf.c b/t/helper/test-read-cache-perf.c
new file mode 100644
index 0000000000..90176c010a
--- /dev/null
+++ b/t/helper/test-read-cache-perf.c
@@ -0,0 +1,21 @@
+#include "test-tool.h"
+#include "cache.h"
+
+int cmd__read_cache_perf(int argc, const char **argv)
+{
+	struct repository *r = the_repository;
+	int i, cnt = 1;
+
+	if (argc == 2)
+		cnt = strtol(argv[1], NULL, 0);
+	else
+		die("usage: test-tool read-cache-perf [<count>]");
+
+	setup_git_directory();
+	for (i = 0; i < cnt; i++) {
+		repo_read_index(r);
+		discard_index(r->index);
+	}
+
+	return 0;
+}
diff --git a/t/helper/test-read-cache.c b/t/helper/test-read-cache.c
index b52c174acc..ae4b9b70ad 100644
--- a/t/helper/test-read-cache.c
+++ b/t/helper/test-read-cache.c
@@ -5,6 +5,12 @@
 #include "commit.h"
 #include "tree.h"
 #include "sparse-index.h"
+#include "parse-options.h"
+
+static const char *read_cache_usage[] = {
+	"test-tool read-cache [<options>...]",
+	NULL
+};
 
 static void print_cache_entry(struct cache_entry *ce)
 {
@@ -34,49 +40,33 @@ static void print_cache(struct index_state *istate)
 int cmd__read_cache(int argc, const char **argv)
 {
 	struct repository *r = the_repository;
-	int i, cnt = 1;
-	const char *name = NULL;
 	int table = 0, expand = 0;
+	struct option options[] = {
+		OPT_BOOL(0, "table", &table,
+			 "print a dump of the cache"),
+		OPT_BOOL(0, "expand", &expand,
+			 "call ensure_full_index()"),
+		OPT_END()
+	};
+
+	argc = parse_options(argc, argv, "test-tools", options, read_cache_usage, 0);
+	if (argc > 0)
+		usage_msg_opt("Too many arguments.", read_cache_usage, options);
 
 	initialize_the_repository();
 	prepare_repo_settings(r);
 	r->settings.command_requires_full_index = 0;
 
-	for (++argv, --argc; *argv && starts_with(*argv, "--"); ++argv, --argc) {
-		if (skip_prefix(*argv, "--print-and-refresh=", &name))
-			continue;
-		if (!strcmp(*argv, "--table"))
-			table = 1;
-		else if (!strcmp(*argv, "--expand"))
-			expand = 1;
-	}
-
-	if (argc == 1)
-		cnt = strtol(argv[0], NULL, 0);
 	setup_git_directory();
 	git_config(git_default_config, NULL);
+	repo_read_index(r);
 
-	for (i = 0; i < cnt; i++) {
-		repo_read_index(r);
-
-		if (expand)
-			ensure_full_index(r->index);
+	if (expand)
+		ensure_full_index(r->index);
 
-		if (name) {
-			int pos;
+	if (table)
+		print_cache(r->index);
+	discard_index(r->index);
 
-			refresh_index(r->index, REFRESH_QUIET,
-				      NULL, NULL, NULL);
-			pos = index_name_pos(r->index, name, strlen(name));
-			if (pos < 0)
-				die("%s not in index", name);
-			printf("%s is%s up to date\n", name,
-			       ce_uptodate(r->index->cache[pos]) ? "" : " not");
-			write_file(name, "%d\n", i);
-		}
-		if (table)
-			print_cache(r->index);
-		discard_index(r->index);
-	}
 	return 0;
 }
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index c5bd0c6d4c..b0300f70c7 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -54,6 +54,8 @@ static struct test_cmd cmds[] = {
 	{ "progress", cmd__progress },
 	{ "reach", cmd__reach },
 	{ "read-cache", cmd__read_cache },
+	{ "read-cache-again", cmd__read_cache_again },
+	{ "read-cache-perf", cmd__read_cache_perf },
 	{ "read-graph", cmd__read_graph },
 	{ "read-midx", cmd__read_midx },
 	{ "ref-store", cmd__ref_store },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index e8069a3b22..7f451a1eb5 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -43,6 +43,8 @@ int cmd__proc_receive(int argc, const char **argv);
 int cmd__progress(int argc, const char **argv);
 int cmd__reach(int argc, const char **argv);
 int cmd__read_cache(int argc, const char **argv);
+int cmd__read_cache_again(int argc, const char **argv);
+int cmd__read_cache_perf(int argc, const char **argv);
 int cmd__read_graph(int argc, const char **argv);
 int cmd__read_midx(int argc, const char **argv);
 int cmd__ref_store(int argc, const char **argv);
diff --git a/t/perf/p0002-read-cache.sh b/t/perf/p0002-read-cache.sh
index cdd105a594..d0ba5173fb 100755
--- a/t/perf/p0002-read-cache.sh
+++ b/t/perf/p0002-read-cache.sh
@@ -8,7 +8,7 @@ test_perf_default_repo
 
 count=1000
 test_perf "read_cache/discard_cache $count times" "
-	test-tool read-cache $count
+	test-tool read-cache-perf $count
 "
 
 test_done
diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
index 637391c6ce..4c199c16d4 100755
--- a/t/t7519-status-fsmonitor.sh
+++ b/t/t7519-status-fsmonitor.sh
@@ -359,7 +359,7 @@ test_expect_success UNTRACKED_CACHE 'ignore .git changes when invalidating UNTR'
 test_expect_success 'discard_index() also discards fsmonitor info' '
 	test_config core.fsmonitor "$TEST_DIRECTORY/t7519/fsmonitor-all" &&
 	test_might_fail git update-index --refresh &&
-	test-tool read-cache --print-and-refresh=tracked 2 >actual &&
+	test-tool read-cache-again 2 tracked >actual &&
 	printf "tracked is%s up to date\n" "" " not" >expect &&
 	test_cmp expect actual
 '
-- 
2.32.0.rc3.434.gd8aed1f08a7

[PATCH 2/4] test-tools: migrate read-cache-perf to parse_options()

[Ævar Arnfjörð Bjarmason]

Change the newly added (but then mostly copy/pasted) read-cache-perf
to use the parse_options() API. This will make things easier as we add
new options.

Since we check the "cnt = < 1" case now via more idiomatic
post-parse_options() assertions we can move from the for-loop to a
while-loop and ditch the "i" variable.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/helper/test-read-cache-perf.c | 26 ++++++++++++++++++++------
 t/perf/p0002-read-cache.sh      |  2 +-
 2 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/t/helper/test-read-cache-perf.c b/t/helper/test-read-cache-perf.c
index 90176c010a..54ad0c3135 100644
--- a/t/helper/test-read-cache-perf.c
+++ b/t/helper/test-read-cache-perf.c
@@ -1,18 +1,32 @@
 #include "test-tool.h"
 #include "cache.h"
+#include "parse-options.h"
+
+static const char *read_cache_perf_usage[] = {
+	"test-tool read-cache-perf [<options>...]",
+	NULL
+};
 
 int cmd__read_cache_perf(int argc, const char **argv)
 {
 	struct repository *r = the_repository;
-	int i, cnt = 1;
+	int cnt = -1;
+	struct option options[] = {
+		OPT_INTEGER(0, "count", &cnt, "number of passes"),
+		OPT_END()
+	};
 
-	if (argc == 2)
-		cnt = strtol(argv[1], NULL, 0);
-	else
-		die("usage: test-tool read-cache-perf [<count>]");
+	argc = parse_options(argc, argv, "test-tools", options,
+			     read_cache_perf_usage, 0);
+	if (argc > 0)
+		usage_msg_opt("Too many arguments.", read_cache_perf_usage,
+			      options);
+	if (cnt < 1)
+		usage_msg_opt("Need at least one pass.", read_cache_perf_usage,
+			      options);
 
 	setup_git_directory();
-	for (i = 0; i < cnt; i++) {
+	while (cnt--) {
 		repo_read_index(r);
 		discard_index(r->index);
 	}
diff --git a/t/perf/p0002-read-cache.sh b/t/perf/p0002-read-cache.sh
index d0ba5173fb..1762b64865 100755
--- a/t/perf/p0002-read-cache.sh
+++ b/t/perf/p0002-read-cache.sh
@@ -8,7 +8,7 @@ test_perf_default_repo
 
 count=1000
 test_perf "read_cache/discard_cache $count times" "
-	test-tool read-cache-perf $count
+	test-tool read-cache-perf --count=$count
 "
 
 test_done
-- 
2.32.0.rc3.434.gd8aed1f08a7

[PATCH 3/4] test-tools: migrate read-cache-again to parse_options()

[Ævar Arnfjörð Bjarmason]

Change the newly added (but then mostly copy/pasted) read-cache-perf
to use the parse_options() API. I have no plans to further modify
read-cache-again, but making these commands consistent has a value in
and of itself.

Since we check the "cnt = < 1" case now via more idiomatic
post-parse_options() assertions we can move from the for-loop to a
while-loop and ditch the "i" variable.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/helper/test-read-cache-again.c | 28 ++++++++++++++++++++++------
 t/t7519-status-fsmonitor.sh      |  2 +-
 2 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/t/helper/test-read-cache-again.c b/t/helper/test-read-cache-again.c
index 707db036cb..8487f79d04 100644
--- a/t/helper/test-read-cache-again.c
+++ b/t/helper/test-read-cache-again.c
@@ -1,20 +1,36 @@
 #include "test-tool.h"
 #include "cache.h"
+#include "parse-options.h"
+
+static const char *read_cache_again_usage[] = {
+	"test-tool read-cache-again [<options>...] <file>",
+	NULL
+};
 
 int cmd__read_cache_again(int argc, const char **argv)
 {
 	struct repository *r = the_repository;
-	int i, cnt;
+	int cnt = -1;
 	const char *name;
+	struct option options[] = {
+		OPT_INTEGER(0, "count", &cnt, "number of passes"),
+		OPT_END()
+	};
 
-	if (argc != 2)
-		die("usage: test-tool read-cache-again <count> <file>");
-
-	cnt = strtol(argv[0], NULL, 0);
+	argc = parse_options(argc, argv, "test-tools", options,
+			     read_cache_again_usage, 0);
+	if (argc != 1)
+		usage_msg_opt("Too many arguments.", read_cache_again_usage,
+			      options);
+	if (cnt == -1)
+		cnt = 2;
+	else if (cnt < 1)
+		usage_msg_opt("Need at least one pass.", read_cache_again_usage,
+			      options);
 	name = argv[2];
 
 	setup_git_directory();
-	for (i = 0; i < cnt; i++) {
+	while (cnt--) {
 		int pos;
 		repo_read_index(r);
 		refresh_index(r->index, REFRESH_QUIET,
diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
index 4c199c16d4..fd0815f6b7 100755
--- a/t/t7519-status-fsmonitor.sh
+++ b/t/t7519-status-fsmonitor.sh
@@ -359,7 +359,7 @@ test_expect_success UNTRACKED_CACHE 'ignore .git changes when invalidating UNTR'
 test_expect_success 'discard_index() also discards fsmonitor info' '
 	test_config core.fsmonitor "$TEST_DIRECTORY/t7519/fsmonitor-all" &&
 	test_might_fail git update-index --refresh &&
-	test-tool read-cache-again 2 tracked >actual &&
+	test-tool read-cache-again --count=2 tracked >actual &&
 	printf "tracked is%s up to date\n" "" " not" >expect &&
 	test_cmp expect actual
 '
-- 
2.32.0.rc3.434.gd8aed1f08a7

[PATCH 4/4] read-cache perf: add a perf test for refresh_index()

[Ævar Arnfjörð Bjarmason]

Add a perf test for the refresh_index() function to compliment the
existing read()/discard() in a loop perf test added in
1ecb5ff141f (read-cache: add simple performance test, 2013-06-09).

Since this test is much slower (around 10x) than the previous
read()/discard() test let's run it 100 times instead of the 1000 time
the first one runs.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/helper/test-read-cache-perf.c | 12 ++++++++++++
 t/perf/p0002-read-cache.sh      |  5 +++++
 2 files changed, 17 insertions(+)

diff --git a/t/helper/test-read-cache-perf.c b/t/helper/test-read-cache-perf.c
index 54ad0c3135..a2802559a5 100644
--- a/t/helper/test-read-cache-perf.c
+++ b/t/helper/test-read-cache-perf.c
@@ -11,8 +11,11 @@ int cmd__read_cache_perf(int argc, const char **argv)
 {
 	struct repository *r = the_repository;
 	int cnt = -1;
+	int refresh = 0;
 	struct option options[] = {
 		OPT_INTEGER(0, "count", &cnt, "number of passes"),
+		OPT_BOOL(0, "refresh", &refresh,
+			 "call refresh_index() in a loop, not read()/discard()"),
 		OPT_END()
 	};
 
@@ -26,10 +29,19 @@ int cmd__read_cache_perf(int argc, const char **argv)
 			      options);
 
 	setup_git_directory();
+	if (refresh)
+		repo_read_index(r);
 	while (cnt--) {
+		if (refresh) {
+			unsigned int flags = REFRESH_QUIET|REFRESH_PROGRESS;
+			refresh_index(r->index, flags, NULL, NULL, NULL);
+			continue;
+		}
 		repo_read_index(r);
 		discard_index(r->index);
 	}
+	if (refresh)
+		discard_index(r->index);
 
 	return 0;
 }
diff --git a/t/perf/p0002-read-cache.sh b/t/perf/p0002-read-cache.sh
index 1762b64865..cbccc5ace9 100755
--- a/t/perf/p0002-read-cache.sh
+++ b/t/perf/p0002-read-cache.sh
@@ -11,4 +11,9 @@ test_perf "read_cache/discard_cache $count times" "
 	test-tool read-cache-perf --count=$count
 "
 
+count=100
+test_perf "refresh_index() $count times" "
+	test-tool read-cache-perf --count=$count --refresh
+"
+
 test_done
-- 
2.32.0.rc3.434.gd8aed1f08a7

Re: [PATCH 0/4] test-tool: split up "read-cache" tool

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> When the sparse index topic was being discussed I suggested that the
> t/helper/read-cache.c tool was getting to the point of doing too many
> things and should be split up.
>
> Since that series has landed on master here's that suggestion again in
> the form of patches on top of master. The 4/4 patch is a "while I was
> at it" addition of an extra perf test for index refreshing.
>
> 1. https://lore.kernel.org/git/20210317132814.30175-6-avarab@gmail.com/
>
> Ævar Arnfjörð Bjarmason (4):
>   test-tool: split up test-tool read-cache
>   test-tools: migrate read-cache-perf to parse_options()
>   test-tools: migrate read-cache-again to parse_options()
>   read-cache perf: add a perf test for refresh_index()

Is the contrast between tool and tools deliberate?


>  Makefile                         |  2 ++
>  t/helper/test-read-cache-again.c | 47 +++++++++++++++++++++++++++
>  t/helper/test-read-cache-perf.c  | 47 +++++++++++++++++++++++++++
>  t/helper/test-read-cache.c       | 56 +++++++++++++-------------------
>  t/helper/test-tool.c             |  2 ++
>  t/helper/test-tool.h             |  2 ++
>  t/perf/p0002-read-cache.sh       |  7 +++-
>  t/t7519-status-fsmonitor.sh      |  2 +-
>  8 files changed, 130 insertions(+), 35 deletions(-)
>  create mode 100644 t/helper/test-read-cache-again.c
>  create mode 100644 t/helper/test-read-cache-perf.c

Re: [PATCH 0/4] test-tool: split up "read-cache" tool

[Ævar Arnfjörð Bjarmason]

On Tue, Jun 08 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> When the sparse index topic was being discussed I suggested that the
>> t/helper/read-cache.c tool was getting to the point of doing too many
>> things and should be split up.
>>
>> Since that series has landed on master here's that suggestion again in
>> the form of patches on top of master. The 4/4 patch is a "while I was
>> at it" addition of an extra perf test for index refreshing.
>>
>> 1. https://lore.kernel.org/git/20210317132814.30175-6-avarab@gmail.com/
>>
>> Ævar Arnfjörð Bjarmason (4):
>>   test-tool: split up test-tool read-cache
>>   test-tools: migrate read-cache-perf to parse_options()
>>   test-tools: migrate read-cache-again to parse_options()
>>   read-cache perf: add a perf test for refresh_index()
>
> Is the contrast between tool and tools deliberate?

Yes, I figured "test-tool:" describes the main entry point of the
"test-tool somecmd", so if I split up "somecmd" into "othercmd" that's a
"test-tool" change.

But "test-tools:" when I'm modifying particular tools, I can change 2/4
and 3/4 it to "test-tool read-cache-perf:" and "test-tool
read-cache-again" (or another thing you suggest) if you think this
warrants a re-roll.

>>  Makefile                         |  2 ++
>>  t/helper/test-read-cache-again.c | 47 +++++++++++++++++++++++++++
>>  t/helper/test-read-cache-perf.c  | 47 +++++++++++++++++++++++++++
>>  t/helper/test-read-cache.c       | 56 +++++++++++++-------------------
>>  t/helper/test-tool.c             |  2 ++
>>  t/helper/test-tool.h             |  2 ++
>>  t/perf/p0002-read-cache.sh       |  7 +++-
>>  t/t7519-status-fsmonitor.sh      |  2 +-
>>  8 files changed, 130 insertions(+), 35 deletions(-)
>>  create mode 100644 t/helper/test-read-cache-again.c
>>  create mode 100644 t/helper/test-read-cache-perf.c

Re: [PATCH 0/4] test-tool: split up "read-cache" tool

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

>>> Ævar Arnfjörð Bjarmason (4):
>>>   test-tool: split up test-tool read-cache
>>>   test-tools: migrate read-cache-perf to parse_options()
>>>   test-tools: migrate read-cache-again to parse_options()
>>>   read-cache perf: add a perf test for refresh_index()
>>
>> Is the contrast between tool and tools deliberate?
>
> Yes, I figured "test-tool:" describes the main entry point of the
> "test-tool somecmd", so if I split up "somecmd" into "othercmd" that's a
> "test-tool" change.
>
> But "test-tools:" when I'm modifying particular tools, I can change 2/4
> and 3/4 it to "test-tool read-cache-perf:" and "test-tool
> read-cache-again" (or another thing you suggest) if you think this
> warrants a re-roll.

I actually meant s/test-tools:/test-tool:/ and nothing else, as
changes to read-cache-perf and changes to read-cache-again both fall
into the same test-tool umbrella.  It's not like we benefit from
having two separate <area> (as in "<area>: <description>")
designators for read-cache-perf and read-cache-again---in a larger
picture, they are both things around test-tool helper.

Thanks.

Re: [PATCH 0/4] test-tool: split up "read-cache" tool

[Emily Shaffer]

On Mon, Jun 07, 2021 at 01:58:23PM +0200, Ævar Arnfjörð Bjarmason wrote:
> 
> When the sparse index topic was being discussed I suggested that the
> t/helper/read-cache.c tool was getting to the point of doing too many
> things and should be split up.
> 
> Since that series has landed on master here's that suggestion again in
> the form of patches on top of master. The 4/4 patch is a "while I was
> at it" addition of an extra perf test for index refreshing.
> 
> 1. https://lore.kernel.org/git/20210317132814.30175-6-avarab@gmail.com/
> 

Since "What's cooking" mentioned this series was starved for review, we
took a look at it today. Most of my comments are pretty general, so I'll
reply primarily to the cover letter instead.

The result after this series is that we have three forms of 'test-tool
read-cache':
 - one for unit testing, with no iteration (test-tool read-cache)
 - one for not-so-perfy iteration (test-tool read-cache-again)
 - one for perfy iteration and refresh_index benching (test-tool
   read-cache-perf)

Before I read patch 4, I said, "'again' and 'perf' have a lot of code in
common, but I guess we are trying to reduce the amount of overhead for
tight-loop performance testing, so OK." But patch 4 adds an alternative
body for the inside of the loop, which *looks* not very performant
(although is probably optimized well) by checking an unchanging bool on
every iteration of the loop.

I tend to think it would be easier to read
and understand to have two forms of 'test-tool read-cache' - one which
iterates and one which does not. Maybe the one that iterates should be
called -perf, maybe it should be called something else, whatever. And
perhaps it makes sense for the iterating one to look like so (heavily
pseudocoded, hope you can follow along with the rough sketch):

  enum iteration_mode;

  parse_options();
  if (should_do_again) {
    iteration_mode = AGAIN;
  else if (should_do_perf) {
    iteration_mode = PERF;
  else if (should_do_refresh) {
    iteration_mode = REFRESH;
  }

  while (passes--)
    switch (iteration_mode) {
    case AGAIN:
      read index;
      refresh index;
      index_name_pos;
      error reporting;
      write_file;
      discard_index;
      break;
    case PERF:
      read index;
      discard index;
      break;
    case REFRESH:
      refresh_index;
      break;
    }

This would put all our "loop lots of times for performance benchmarking"
into one place. We know that the switch statement is very performant,
especially if we manage to const-ify iteration_mode.  The cases make it
very clear that the body of the loop is being swapped out depending on
the arguments, and that entirely different behavior is happening in each
scenario.

There's also an orthogonal bit of cleanup here by moving to
parse_options(), which I am excited about in general :) but which I
think wasn't done very cleanly in this series. In patch 1, the commit
message makes no mention of the fairly significant refactor happening to
move 'test-tool read-cache' to parse_options(), and I think the mix of
cut&paste with refactor makes the patch a little muddy. What about a
series like so:

 1/3: teach test-tool read-cache to use parse_options()
 2/3: add test-tool read-cache-(perf|iterating|whatever)
 3/3: teach test-tool read-cache-perf "--refresh"


Thanks, and hopefully this is a welcome necromancy and not an annoying
one ;)

 - Emily

Re: [PATCH 0/4] test-tool: split up "read-cache" tool

[Ævar Arnfjörð Bjarmason]

On Wed, Jun 30 2021, Emily Shaffer wrote:

> On Mon, Jun 07, 2021 at 01:58:23PM +0200, Ævar Arnfjörð Bjarmason wrote:
>> 
>> When the sparse index topic was being discussed I suggested that the
>> t/helper/read-cache.c tool was getting to the point of doing too many
>> things and should be split up.
>> 
>> Since that series has landed on master here's that suggestion again in
>> the form of patches on top of master. The 4/4 patch is a "while I was
>> at it" addition of an extra perf test for index refreshing.
>> 
>> 1. https://lore.kernel.org/git/20210317132814.30175-6-avarab@gmail.com/
>> 
>
> Since "What's cooking" mentioned this series was starved for review, we
> took a look at it today. Most of my comments are pretty general, so I'll
> reply primarily to the cover letter instead.
>
> The result after this series is that we have three forms of 'test-tool
> read-cache':
>  - one for unit testing, with no iteration (test-tool read-cache)
>  - one for not-so-perfy iteration (test-tool read-cache-again)
>  - one for perfy iteration and refresh_index benching (test-tool
>    read-cache-perf)
>
> Before I read patch 4, I said, "'again' and 'perf' have a lot of code in
> common, but I guess we are trying to reduce the amount of overhead for
> tight-loop performance testing, so OK." But patch 4 adds an alternative
> body for the inside of the loop, which *looks* not very performant
> (although is probably optimized well) by checking an unchanging bool on
> every iteration of the loop.
>
> I tend to think it would be easier to read
> and understand to have two forms of 'test-tool read-cache' - one which
> iterates and one which does not. Maybe the one that iterates should be
> called -perf, maybe it should be called something else, whatever. And
> perhaps it makes sense for the iterating one to look like so (heavily
> pseudocoded, hope you can follow along with the rough sketch):
>
>   enum iteration_mode;
>
>   parse_options();
>   if (should_do_again) {
>     iteration_mode = AGAIN;
>   else if (should_do_perf) {
>     iteration_mode = PERF;
>   else if (should_do_refresh) {
>     iteration_mode = REFRESH;
>   }
>
>   while (passes--)
>     switch (iteration_mode) {
>     case AGAIN:
>       read index;
>       refresh index;
>       index_name_pos;
>       error reporting;
>       write_file;
>       discard_index;
>       break;
>     case PERF:
>       read index;
>       discard index;
>       break;
>     case REFRESH:
>       refresh_index;
>       break;
>     }
>
> This would put all our "loop lots of times for performance benchmarking"
> into one place. We know that the switch statement is very performant,
> especially if we manage to const-ify iteration_mode.  The cases make it
> very clear that the body of the loop is being swapped out depending on
> the arguments, and that entirely different behavior is happening in each
> scenario.
>
> There's also an orthogonal bit of cleanup here by moving to
> parse_options(), which I am excited about in general :) but which I
> think wasn't done very cleanly in this series. In patch 1, the commit
> message makes no mention of the fairly significant refactor happening to
> move 'test-tool read-cache' to parse_options(), and I think the mix of
> cut&paste with refactor makes the patch a little muddy. What about a
> series like so:
>
>  1/3: teach test-tool read-cache to use parse_options()
>  2/3: add test-tool read-cache-(perf|iterating|whatever)
>  3/3: teach test-tool read-cache-perf "--refresh"
>
>
> Thanks, and hopefully this is a welcome necromancy and not an annoying
> one ;)

Thanks.

Yes 1/4 is a bit confsing, but the alternative to converting it to
parse_option while we're at it would be to introduce support for
understanding "print-and-refresh" in one commit, only to remove it in
another. So I aimed for something that would play as well with the diff
rename detection as possible for 1/4. Do you think it's worth it to add
such an back & forth step?

For the rest I think there's been a bit of a misunderstanding (which is
on me in not explaining this well enough), i.e. your:

    I guess we are trying to reduce the amount of overhead for
    tight-loop performance testing

That wasn't really what I was going for at all, but rather (copied from
the 1st commit message):

    I think that having one test tool do so many different things makes it
    harder to read its code. Let's instead split up the "again" and "perf"
    uses for it into their own tools.

This series was extracted out of an RFC-reply of mine to one of
Derrick's recent patches where he was adding more knobs to
test-read-cache.c (and I understand the plan in the near-term is to add
more sparse-specific things to it).

So the goal here was only to split these different use cases all sharing
one function so that they used 3 functions instead.

Your suggestion of the switch/case loop would work, and would be
performant. Optimizing this wasn't a goal at all though, making it more
readable was.

I don't think it matters much if at all how performant the scaffolding
of the test-tool itself is, if it was relatively more expensive we could
just run more loops and we'd eventually get meaningful numbers in the
perf test. The "perf" in the name is just "this is for the perf test"
not "this is the performant version".

I think that for the test tools where we're mostly not aiming for
generalized but helpers narrowly tailored to specific tests it makes
more sense to split early than have tools that take N options, with
functions that use some small subset of those N options for any given
use-case.

There's going to be duplication in the scaffolding
(e.g. parse_options()), but that sort of thing is easy to read, whereas
reasoning about some inner loop and wondering "hrm, did this config call
there impact any of this" takes more time than it should.

Anyway, knowing that's the goal (and performance of the tool itself
wasn't, at all), do you think the end result makes more/less sense than
before?

[PATCH v2 0/4] test-tool: split up "read-cache" tool

[Ævar Arnfjörð Bjarmason]

A re-roll addressing the feedback v1 got, see
https://lore.kernel.org/git/cover-0.4-0000000000-20210607T115454Z-avarab@gmail.com

I think the gist of that feedback from Emily was that v1 could be
understood as aiming to optimize the perf test, but the main reason
I'm doing this split up is to make the code easier to read. The
changed commit message summarizes both goals.

I also changed some nits in the code I spotted myself, e.g. "argc > 0"
checks to just "argc", and a simpler way of providing the "cnt"
default.

Ævar Arnfjörð Bjarmason (4):
  test-tool: split up test-tool read-cache
  test-tool: migrate read-cache-perf to parse_options()
  test-tool: migrate read-cache-again to parse_options()
  read-cache perf: add a perf test for refresh_index()

 Makefile                         |  2 ++
 t/helper/test-read-cache-again.c | 45 +++++++++++++++++++++++++
 t/helper/test-read-cache-perf.c  | 47 +++++++++++++++++++++++++++
 t/helper/test-read-cache.c       | 56 +++++++++++++-------------------
 t/helper/test-tool.c             |  2 ++
 t/helper/test-tool.h             |  2 ++
 t/perf/p0002-read-cache.sh       |  7 +++-
 t/t7519-status-fsmonitor.sh      |  2 +-
 8 files changed, 128 insertions(+), 35 deletions(-)
 create mode 100644 t/helper/test-read-cache-again.c
 create mode 100644 t/helper/test-read-cache-perf.c

Range-diff against v1:
1:  6e7fcd46934 ! 1:  adb3f989a29 test-tool: split up test-tool read-cache
    @@ Commit message
         test-tool: split up test-tool read-cache
     
         Since the "test-tool read-cache" was originally added back in
    -    1ecb5ff141 (read-cache: add simple performance test, 2013-06-09) it's
    -    been growing all sorts of bells and whistles that aren't very
    -    conducive to performance testing the index, e.g. it learned how to
    -    read config.
    +    1ecb5ff141 (read-cache: add simple performance test, 2013-06-09) the
    +    test-read-cache.c tool has been growing various features that make the
    +    code harder to read. I.e. sometimes running as a one-off, sometimes looping.
    +
    +    It's also been unconditionally reading config since
    +    dc76852df2f (fsmonitor: demonstrate that it is not refreshed after
    +    discard_index(), 2019-05-07), which introduces unnecessary noise into
    +    the performance test.
     
         Then in recent changes in e2df6c3972 (test-read-cache: print cache
         entries with --table, 2021-03-30) and 2782db3eed (test-tool: don't
    @@ Commit message
     
         I think that having one test tool do so many different things makes it
         harder to read its code. Let's instead split up the "again" and "perf"
    -    uses for it into their own tools.
    +    uses for it into their own small tools, this makes the main
    +    "test-read-cache.c" a simpler.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    @@ t/helper/test-read-cache.c: static void print_cache(struct index_state *istate)
     +	};
     +
     +	argc = parse_options(argc, argv, "test-tools", options, read_cache_usage, 0);
    -+	if (argc > 0)
    ++	if (argc)
     +		usage_msg_opt("Too many arguments.", read_cache_usage, options);
      
      	initialize_the_repository();
2:  07f392e0878 ! 2:  a68fa4a6355 test-tools: migrate read-cache-perf to parse_options()
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Commit message ##
    -    test-tools: migrate read-cache-perf to parse_options()
    +    test-tool: migrate read-cache-perf to parse_options()
     
         Change the newly added (but then mostly copy/pasted) read-cache-perf
         to use the parse_options() API. This will make things easier as we add
    @@ t/helper/test-read-cache-perf.c
     -		die("usage: test-tool read-cache-perf [<count>]");
     +	argc = parse_options(argc, argv, "test-tools", options,
     +			     read_cache_perf_usage, 0);
    -+	if (argc > 0)
    ++	if (argc)
     +		usage_msg_opt("Too many arguments.", read_cache_perf_usage,
     +			      options);
     +	if (cnt < 1)
3:  36f4072b131 ! 3:  a34e69eaa48 test-tools: migrate read-cache-again to parse_options()
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Commit message ##
    -    test-tools: migrate read-cache-again to parse_options()
    +    test-tool: migrate read-cache-again to parse_options()
     
         Change the newly added (but then mostly copy/pasted) read-cache-perf
         to use the parse_options() API. I have no plans to further modify
    @@ t/helper/test-read-cache-again.c
      {
      	struct repository *r = the_repository;
     -	int i, cnt;
    -+	int cnt = -1;
    ++	int cnt = 2;
      	const char *name;
     +	struct option options[] = {
     +		OPT_INTEGER(0, "count", &cnt, "number of passes"),
    @@ t/helper/test-read-cache-again.c
     +	if (argc != 1)
     +		usage_msg_opt("Too many arguments.", read_cache_again_usage,
     +			      options);
    -+	if (cnt == -1)
    -+		cnt = 2;
    -+	else if (cnt < 1)
    ++	if (cnt < 1)
     +		usage_msg_opt("Need at least one pass.", read_cache_again_usage,
     +			      options);
      	name = argv[2];
4:  120a37acaef = 4:  e3648bf78c7 read-cache perf: add a perf test for refresh_index()
-- 
2.33.0.663.gbaff4edb973

[PATCH v2 1/4] test-tool: split up test-tool read-cache

[Ævar Arnfjörð Bjarmason]

Since the "test-tool read-cache" was originally added back in
1ecb5ff141 (read-cache: add simple performance test, 2013-06-09) the
test-read-cache.c tool has been growing various features that make the
code harder to read. I.e. sometimes running as a one-off, sometimes looping.

It's also been unconditionally reading config since
dc76852df2f (fsmonitor: demonstrate that it is not refreshed after
discard_index(), 2019-05-07), which introduces unnecessary noise into
the performance test.

Then in recent changes in e2df6c3972 (test-read-cache: print cache
entries with --table, 2021-03-30) and 2782db3eed (test-tool: don't
force full index, 2021-03-30) we gained even more logic to deal with
sparse index testing.

I think that having one test tool do so many different things makes it
harder to read its code. Let's instead split up the "again" and "perf"
uses for it into their own small tools, this makes the main
"test-read-cache.c" a simpler.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile                         |  2 ++
 t/helper/test-read-cache-again.c | 31 ++++++++++++++++++
 t/helper/test-read-cache-perf.c  | 21 ++++++++++++
 t/helper/test-read-cache.c       | 56 +++++++++++++-------------------
 t/helper/test-tool.c             |  2 ++
 t/helper/test-tool.h             |  2 ++
 t/perf/p0002-read-cache.sh       |  2 +-
 t/t7519-status-fsmonitor.sh      |  2 +-
 8 files changed, 83 insertions(+), 35 deletions(-)
 create mode 100644 t/helper/test-read-cache-again.c
 create mode 100644 t/helper/test-read-cache-perf.c

diff --git a/Makefile b/Makefile
index 9573190f1d7..4d3f8ece74f 100644
--- a/Makefile
+++ b/Makefile
@@ -739,6 +739,8 @@ TEST_BUILTINS_OBJS += test-prio-queue.o
 TEST_BUILTINS_OBJS += test-proc-receive.o
 TEST_BUILTINS_OBJS += test-progress.o
 TEST_BUILTINS_OBJS += test-reach.o
+TEST_BUILTINS_OBJS += test-read-cache-again.o
+TEST_BUILTINS_OBJS += test-read-cache-perf.o
 TEST_BUILTINS_OBJS += test-read-cache.o
 TEST_BUILTINS_OBJS += test-read-graph.o
 TEST_BUILTINS_OBJS += test-read-midx.o
diff --git a/t/helper/test-read-cache-again.c b/t/helper/test-read-cache-again.c
new file mode 100644
index 00000000000..707db036cb1
--- /dev/null
+++ b/t/helper/test-read-cache-again.c
@@ -0,0 +1,31 @@
+#include "test-tool.h"
+#include "cache.h"
+
+int cmd__read_cache_again(int argc, const char **argv)
+{
+	struct repository *r = the_repository;
+	int i, cnt;
+	const char *name;
+
+	if (argc != 2)
+		die("usage: test-tool read-cache-again <count> <file>");
+
+	cnt = strtol(argv[0], NULL, 0);
+	name = argv[2];
+
+	setup_git_directory();
+	for (i = 0; i < cnt; i++) {
+		int pos;
+		repo_read_index(r);
+		refresh_index(r->index, REFRESH_QUIET,
+			      NULL, NULL, NULL);
+		pos = index_name_pos(r->index, name, strlen(name));
+		if (pos < 0)
+			die("%s not in index", name);
+		printf("%s is%s up to date\n", name,
+		       ce_uptodate(r->index->cache[pos]) ? "" : " not");
+		write_file(name, "%d\n", cnt);
+		discard_index(r->index);
+	}
+	return 0;
+}
diff --git a/t/helper/test-read-cache-perf.c b/t/helper/test-read-cache-perf.c
new file mode 100644
index 00000000000..90176c010a1
--- /dev/null
+++ b/t/helper/test-read-cache-perf.c
@@ -0,0 +1,21 @@
+#include "test-tool.h"
+#include "cache.h"
+
+int cmd__read_cache_perf(int argc, const char **argv)
+{
+	struct repository *r = the_repository;
+	int i, cnt = 1;
+
+	if (argc == 2)
+		cnt = strtol(argv[1], NULL, 0);
+	else
+		die("usage: test-tool read-cache-perf [<count>]");
+
+	setup_git_directory();
+	for (i = 0; i < cnt; i++) {
+		repo_read_index(r);
+		discard_index(r->index);
+	}
+
+	return 0;
+}
diff --git a/t/helper/test-read-cache.c b/t/helper/test-read-cache.c
index b52c174acc7..616894072d1 100644
--- a/t/helper/test-read-cache.c
+++ b/t/helper/test-read-cache.c
@@ -5,6 +5,12 @@
 #include "commit.h"
 #include "tree.h"
 #include "sparse-index.h"
+#include "parse-options.h"
+
+static const char *read_cache_usage[] = {
+	"test-tool read-cache [<options>...]",
+	NULL
+};
 
 static void print_cache_entry(struct cache_entry *ce)
 {
@@ -34,49 +40,33 @@ static void print_cache(struct index_state *istate)
 int cmd__read_cache(int argc, const char **argv)
 {
 	struct repository *r = the_repository;
-	int i, cnt = 1;
-	const char *name = NULL;
 	int table = 0, expand = 0;
+	struct option options[] = {
+		OPT_BOOL(0, "table", &table,
+			 "print a dump of the cache"),
+		OPT_BOOL(0, "expand", &expand,
+			 "call ensure_full_index()"),
+		OPT_END()
+	};
+
+	argc = parse_options(argc, argv, "test-tools", options, read_cache_usage, 0);
+	if (argc)
+		usage_msg_opt("Too many arguments.", read_cache_usage, options);
 
 	initialize_the_repository();
 	prepare_repo_settings(r);
 	r->settings.command_requires_full_index = 0;
 
-	for (++argv, --argc; *argv && starts_with(*argv, "--"); ++argv, --argc) {
-		if (skip_prefix(*argv, "--print-and-refresh=", &name))
-			continue;
-		if (!strcmp(*argv, "--table"))
-			table = 1;
-		else if (!strcmp(*argv, "--expand"))
-			expand = 1;
-	}
-
-	if (argc == 1)
-		cnt = strtol(argv[0], NULL, 0);
 	setup_git_directory();
 	git_config(git_default_config, NULL);
+	repo_read_index(r);
 
-	for (i = 0; i < cnt; i++) {
-		repo_read_index(r);
-
-		if (expand)
-			ensure_full_index(r->index);
+	if (expand)
+		ensure_full_index(r->index);
 
-		if (name) {
-			int pos;
+	if (table)
+		print_cache(r->index);
+	discard_index(r->index);
 
-			refresh_index(r->index, REFRESH_QUIET,
-				      NULL, NULL, NULL);
-			pos = index_name_pos(r->index, name, strlen(name));
-			if (pos < 0)
-				die("%s not in index", name);
-			printf("%s is%s up to date\n", name,
-			       ce_uptodate(r->index->cache[pos]) ? "" : " not");
-			write_file(name, "%d\n", i);
-		}
-		if (table)
-			print_cache(r->index);
-		discard_index(r->index);
-	}
 	return 0;
 }
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index 3ce5585e53a..fb537c158e9 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -57,6 +57,8 @@ static struct test_cmd cmds[] = {
 	{ "progress", cmd__progress },
 	{ "reach", cmd__reach },
 	{ "read-cache", cmd__read_cache },
+	{ "read-cache-again", cmd__read_cache_again },
+	{ "read-cache-perf", cmd__read_cache_perf },
 	{ "read-graph", cmd__read_graph },
 	{ "read-midx", cmd__read_midx },
 	{ "ref-store", cmd__ref_store },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index 9f0f5228508..e5616e6f7a7 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -46,6 +46,8 @@ int cmd__proc_receive(int argc, const char **argv);
 int cmd__progress(int argc, const char **argv);
 int cmd__reach(int argc, const char **argv);
 int cmd__read_cache(int argc, const char **argv);
+int cmd__read_cache_again(int argc, const char **argv);
+int cmd__read_cache_perf(int argc, const char **argv);
 int cmd__read_graph(int argc, const char **argv);
 int cmd__read_midx(int argc, const char **argv);
 int cmd__ref_store(int argc, const char **argv);
diff --git a/t/perf/p0002-read-cache.sh b/t/perf/p0002-read-cache.sh
index cdd105a5945..d0ba5173fb1 100755
--- a/t/perf/p0002-read-cache.sh
+++ b/t/perf/p0002-read-cache.sh
@@ -8,7 +8,7 @@ test_perf_default_repo
 
 count=1000
 test_perf "read_cache/discard_cache $count times" "
-	test-tool read-cache $count
+	test-tool read-cache-perf $count
 "
 
 test_done
diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
index deea88d4431..7de5fcb1bd7 100755
--- a/t/t7519-status-fsmonitor.sh
+++ b/t/t7519-status-fsmonitor.sh
@@ -360,7 +360,7 @@ test_expect_success UNTRACKED_CACHE 'ignore .git changes when invalidating UNTR'
 test_expect_success 'discard_index() also discards fsmonitor info' '
 	test_config core.fsmonitor "$TEST_DIRECTORY/t7519/fsmonitor-all" &&
 	test_might_fail git update-index --refresh &&
-	test-tool read-cache --print-and-refresh=tracked 2 >actual &&
+	test-tool read-cache-again 2 tracked >actual &&
 	printf "tracked is%s up to date\n" "" " not" >expect &&
 	test_cmp expect actual
 '
-- 
2.33.0.663.gbaff4edb973

[PATCH v2 2/4] test-tool: migrate read-cache-perf to parse_options()

[Ævar Arnfjörð Bjarmason]

Change the newly added (but then mostly copy/pasted) read-cache-perf
to use the parse_options() API. This will make things easier as we add
new options.

Since we check the "cnt = < 1" case now via more idiomatic
post-parse_options() assertions we can move from the for-loop to a
while-loop and ditch the "i" variable.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/helper/test-read-cache-perf.c | 26 ++++++++++++++++++++------
 t/perf/p0002-read-cache.sh      |  2 +-
 2 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/t/helper/test-read-cache-perf.c b/t/helper/test-read-cache-perf.c
index 90176c010a1..301e98797b4 100644
--- a/t/helper/test-read-cache-perf.c
+++ b/t/helper/test-read-cache-perf.c
@@ -1,18 +1,32 @@
 #include "test-tool.h"
 #include "cache.h"
+#include "parse-options.h"
+
+static const char *read_cache_perf_usage[] = {
+	"test-tool read-cache-perf [<options>...]",
+	NULL
+};
 
 int cmd__read_cache_perf(int argc, const char **argv)
 {
 	struct repository *r = the_repository;
-	int i, cnt = 1;
+	int cnt = -1;
+	struct option options[] = {
+		OPT_INTEGER(0, "count", &cnt, "number of passes"),
+		OPT_END()
+	};
 
-	if (argc == 2)
-		cnt = strtol(argv[1], NULL, 0);
-	else
-		die("usage: test-tool read-cache-perf [<count>]");
+	argc = parse_options(argc, argv, "test-tools", options,
+			     read_cache_perf_usage, 0);
+	if (argc)
+		usage_msg_opt("Too many arguments.", read_cache_perf_usage,
+			      options);
+	if (cnt < 1)
+		usage_msg_opt("Need at least one pass.", read_cache_perf_usage,
+			      options);
 
 	setup_git_directory();
-	for (i = 0; i < cnt; i++) {
+	while (cnt--) {
 		repo_read_index(r);
 		discard_index(r->index);
 	}
diff --git a/t/perf/p0002-read-cache.sh b/t/perf/p0002-read-cache.sh
index d0ba5173fb1..1762b648654 100755
--- a/t/perf/p0002-read-cache.sh
+++ b/t/perf/p0002-read-cache.sh
@@ -8,7 +8,7 @@ test_perf_default_repo
 
 count=1000
 test_perf "read_cache/discard_cache $count times" "
-	test-tool read-cache-perf $count
+	test-tool read-cache-perf --count=$count
 "
 
 test_done
-- 
2.33.0.663.gbaff4edb973

[PATCH v2 3/4] test-tool: migrate read-cache-again to parse_options()

[Ævar Arnfjörð Bjarmason]

Change the newly added (but then mostly copy/pasted) read-cache-perf
to use the parse_options() API. I have no plans to further modify
read-cache-again, but making these commands consistent has a value in
and of itself.

Since we check the "cnt = < 1" case now via more idiomatic
post-parse_options() assertions we can move from the for-loop to a
while-loop and ditch the "i" variable.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/helper/test-read-cache-again.c | 26 ++++++++++++++++++++------
 t/t7519-status-fsmonitor.sh      |  2 +-
 2 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/t/helper/test-read-cache-again.c b/t/helper/test-read-cache-again.c
index 707db036cb1..fa456ca229e 100644
--- a/t/helper/test-read-cache-again.c
+++ b/t/helper/test-read-cache-again.c
@@ -1,20 +1,34 @@
 #include "test-tool.h"
 #include "cache.h"
+#include "parse-options.h"
+
+static const char *read_cache_again_usage[] = {
+	"test-tool read-cache-again [<options>...] <file>",
+	NULL
+};
 
 int cmd__read_cache_again(int argc, const char **argv)
 {
 	struct repository *r = the_repository;
-	int i, cnt;
+	int cnt = 2;
 	const char *name;
+	struct option options[] = {
+		OPT_INTEGER(0, "count", &cnt, "number of passes"),
+		OPT_END()
+	};
 
-	if (argc != 2)
-		die("usage: test-tool read-cache-again <count> <file>");
-
-	cnt = strtol(argv[0], NULL, 0);
+	argc = parse_options(argc, argv, "test-tools", options,
+			     read_cache_again_usage, 0);
+	if (argc != 1)
+		usage_msg_opt("Too many arguments.", read_cache_again_usage,
+			      options);
+	if (cnt < 1)
+		usage_msg_opt("Need at least one pass.", read_cache_again_usage,
+			      options);
 	name = argv[2];
 
 	setup_git_directory();
-	for (i = 0; i < cnt; i++) {
+	while (cnt--) {
 		int pos;
 		repo_read_index(r);
 		refresh_index(r->index, REFRESH_QUIET,
diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
index 7de5fcb1bd7..817e642f58e 100755
--- a/t/t7519-status-fsmonitor.sh
+++ b/t/t7519-status-fsmonitor.sh
@@ -360,7 +360,7 @@ test_expect_success UNTRACKED_CACHE 'ignore .git changes when invalidating UNTR'
 test_expect_success 'discard_index() also discards fsmonitor info' '
 	test_config core.fsmonitor "$TEST_DIRECTORY/t7519/fsmonitor-all" &&
 	test_might_fail git update-index --refresh &&
-	test-tool read-cache-again 2 tracked >actual &&
+	test-tool read-cache-again --count=2 tracked >actual &&
 	printf "tracked is%s up to date\n" "" " not" >expect &&
 	test_cmp expect actual
 '
-- 
2.33.0.663.gbaff4edb973

[PATCH v2 4/4] read-cache perf: add a perf test for refresh_index()

[Ævar Arnfjörð Bjarmason]

Add a perf test for the refresh_index() function to compliment the
existing read()/discard() in a loop perf test added in
1ecb5ff141f (read-cache: add simple performance test, 2013-06-09).

Since this test is much slower (around 10x) than the previous
read()/discard() test let's run it 100 times instead of the 1000 time
the first one runs.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/helper/test-read-cache-perf.c | 12 ++++++++++++
 t/perf/p0002-read-cache.sh      |  5 +++++
 2 files changed, 17 insertions(+)

diff --git a/t/helper/test-read-cache-perf.c b/t/helper/test-read-cache-perf.c
index 301e98797b4..630c126fbf3 100644
--- a/t/helper/test-read-cache-perf.c
+++ b/t/helper/test-read-cache-perf.c
@@ -11,8 +11,11 @@ int cmd__read_cache_perf(int argc, const char **argv)
 {
 	struct repository *r = the_repository;
 	int cnt = -1;
+	int refresh = 0;
 	struct option options[] = {
 		OPT_INTEGER(0, "count", &cnt, "number of passes"),
+		OPT_BOOL(0, "refresh", &refresh,
+			 "call refresh_index() in a loop, not read()/discard()"),
 		OPT_END()
 	};
 
@@ -26,10 +29,19 @@ int cmd__read_cache_perf(int argc, const char **argv)
 			      options);
 
 	setup_git_directory();
+	if (refresh)
+		repo_read_index(r);
 	while (cnt--) {
+		if (refresh) {
+			unsigned int flags = REFRESH_QUIET|REFRESH_PROGRESS;
+			refresh_index(r->index, flags, NULL, NULL, NULL);
+			continue;
+		}
 		repo_read_index(r);
 		discard_index(r->index);
 	}
+	if (refresh)
+		discard_index(r->index);
 
 	return 0;
 }
diff --git a/t/perf/p0002-read-cache.sh b/t/perf/p0002-read-cache.sh
index 1762b648654..cbccc5ace95 100755
--- a/t/perf/p0002-read-cache.sh
+++ b/t/perf/p0002-read-cache.sh
@@ -11,4 +11,9 @@ test_perf "read_cache/discard_cache $count times" "
 	test-tool read-cache-perf --count=$count
 "
 
+count=100
+test_perf "refresh_index() $count times" "
+	test-tool read-cache-perf --count=$count --refresh
+"
+
 test_done
-- 
2.33.0.663.gbaff4edb973

Re: [PATCH v2 0/4] test-tool: split up "read-cache" tool

[Taylor Blau]

On Tue, Aug 24, 2021 at 11:15:21AM +0200, Ævar Arnfjörð Bjarmason wrote:
> A re-roll addressing the feedback v1 got, see
> https://lore.kernel.org/git/cover-0.4-0000000000-20210607T115454Z-avarab@gmail.com
>
> I think the gist of that feedback from Emily was that v1 could be
> understood as aiming to optimize the perf test, but the main reason
> I'm doing this split up is to make the code easier to read. The
> changed commit message summarizes both goals.
>
> I also changed some nits in the code I spotted myself, e.g. "argc > 0"
> checks to just "argc", and a simpler way of providing the "cnt"
> default.

Playing devil's advocate for a moment: I think that the current
implementation is somewhat easier to read as it lives in a single file,
and makes clear that each option implies different behavior through the
body of the loop.

If we're looking for things to clean up, I do like the conversion to the
parse-options API instead of reading argv ourselves, but probably
otherwise prefer the code as-is instead of split across many files.

But I may be in the minority, and there may be others who do find the
split-up version easier to grok.

Thanks,
Taylor

Re: [PATCH v2 0/4] test-tool: split up "read-cache" tool

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> If we're looking for things to clean up, I do like the conversion to the
> parse-options API instead of reading argv ourselves, but probably
> otherwise prefer the code as-is instead of split across many files.
>
> But I may be in the minority, and there may be others who do find the
> split-up version easier to grok.

FWIW, you're not alone.  I too like the use of parse_options, but I
fail to be enthused by changes to churn the test helper binary.

Re: [PATCH v2 0/4] test-tool: split up "read-cache" tool

[Ævar Arnfjörð Bjarmason]

On Tue, Aug 24 2021, Junio C Hamano wrote:

> Taylor Blau <me@ttaylorr.com> writes:
>
>> If we're looking for things to clean up, I do like the conversion to the
>> parse-options API instead of reading argv ourselves, but probably
>> otherwise prefer the code as-is instead of split across many files.
>>
>> But I may be in the minority, and there may be others who do find the
>> split-up version easier to grok.
>
> FWIW, you're not alone.  I too like the use of parse_options, but I
> fail to be enthused by changes to churn the test helper binary.

I think it's an improvement as-is & would like you to pick it up, but I
understand if you disagree.

This series converts the code to parse_options(), but also changes the
perf test to not load config, and adds a --refresh option for a new perf
test.

I tried to make that readable all in one function, but I think it just
ends up being more of a confusing if/else jungle than the current
state.

I don't think I'm the right person to re-roll this in that way & argue
for it being an improvement when I don't think it would be.