[v5,24/30] t/perf/p7519: speed up test on Windows

Commit Message

Jeff Hostetler Feb. 11, 2022, 8:56 p.m. UTC

From: Jeff Hostetler <[e-mail]>

Change p7519 to use `test_seq` and `xargs` rather than a `for` loop
to touch thousands of files.  This takes minutes off of test runs
on Windows because of process creation overhead.

Signed-off-by: Jeff Hostetler <[e-mail]>
---
 t/perf/p7519-fsmonitor.sh | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

Comments

Junio C Hamano Feb. 17, 2022, 1:15 a.m. UTC | #1

"Jeff Hostetler via GitGitGadget" <[e-mail]> writes:

> +touch_files () {
> +	n=$1
> +	d="$n"_files
> +
> +	(cd $d ; test_seq 1 $n | xargs touch )
> +}
> +
>  test_expect_success "one time repo setup" '
>  	# set untrackedCache depending on the environment
>  	if test -n "$GIT_PERF_7519_UNTRACKED_CACHE"
> @@ -119,10 +126,11 @@ test_expect_success "one time repo setup" '
>  	fi &&
>  
>  	mkdir 1_file 10_files 100_files 1000_files 10000_files &&
> -	for i in $(test_seq 1 10); do touch 10_files/$i || return 1; done &&
> -	for i in $(test_seq 1 100); do touch 100_files/$i || return 1; done &&
> -	for i in $(test_seq 1 1000); do touch 1000_files/$i || return 1; done &&
> -	for i in $(test_seq 1 10000); do touch 10000_files/$i || return 1; done &&
> +	touch_files 1 &&

This causes touch_files to chdir to 1_files and run "touch 1" in
there, but because there is no such directory (we have 1_file/
directory, but not 1_files/ directory), it would fail.

> +	touch_files 10 &&
> +	touch_files 100 &&
> +	touch_files 1000 &&
> +	touch_files 10000 &&

Apparently nobody has run this perf script recently since part #2
was posted.

>  	git add 1_file 10_files 100_files 1000_files 10000_files &&

The original introduced at bb7cc7e7 (t/perf/fsmonitor: separate one
time repo initialization, 2020-10-26) created an empty directory 1_file
and without creating anything in it, ran "git add" on it.

If we are not doing anything to 1_file directory anyway, perhaps we
can get rid of it to avoid the breakage in "make perf"?

If we have a chance to reroll this series, we can squash in
something like this, perhaps (it does not deserve to be a separate
step).

--- >8 ---
Subject: [PATCH] p7519: leave 1_file directory empty

The step "t/perf/p7519: speed up test on Windows" in the topic
builtin-fsmonitor-part-2 (not in 'next' yet) attempts to create one
file in 1_files directory, but the original introduced at bb7cc7e7
(t/perf/fsmonitor: separate one time repo initialization,
2020-10-26):

 (1) created 1_file directory,

 (2) left the directory empty, and

 (3) a later test expected (and still expects) that there is nothing
     in the directory.

Revert the behaviour back to what the original wanted to do.

Signed-off-by: Junio C Hamano <[e-mail]>
---
 t/perf/p7519-fsmonitor.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/perf/p7519-fsmonitor.sh b/t/perf/p7519-fsmonitor.sh
index 9a2288a622..a1c552129c 100755
--- a/t/perf/p7519-fsmonitor.sh
+++ b/t/perf/p7519-fsmonitor.sh
@@ -126,7 +126,7 @@ test_expect_success "one time repo setup" '
 	fi &&
 
 	mkdir 1_file 10_files 100_files 1000_files 10000_files &&
-	touch_files 1 &&
+	: 1_file directory should be left empty &&
 	touch_files 10 &&
 	touch_files 100 &&
 	touch_files 1000 &&

Jeff Hostetler Feb. 17, 2022, 7:03 p.m. UTC | #2

On 2/16/22 8:15 PM, Junio C Hamano wrote:
> "Jeff Hostetler via GitGitGadget" <[e-mail]> writes:
> 
>> +touch_files () {
>> +	n=$1
>> +	d="$n"_files
>> +
>> +	(cd $d ; test_seq 1 $n | xargs touch )
>> +}
>> +
>>   test_expect_success "one time repo setup" '
>>   	# set untrackedCache depending on the environment
>>   	if test -n "$GIT_PERF_7519_UNTRACKED_CACHE"
>> @@ -119,10 +126,11 @@ test_expect_success "one time repo setup" '
>>   	fi &&
>>   
>>   	mkdir 1_file 10_files 100_files 1000_files 10000_files &&
>> -	for i in $(test_seq 1 10); do touch 10_files/$i || return 1; done &&
>> -	for i in $(test_seq 1 100); do touch 100_files/$i || return 1; done &&
>> -	for i in $(test_seq 1 1000); do touch 1000_files/$i || return 1; done &&
>> -	for i in $(test_seq 1 10000); do touch 10000_files/$i || return 1; done &&
>> +	touch_files 1 &&
> 
> This causes touch_files to chdir to 1_files and run "touch 1" in
> there, but because there is no such directory (we have 1_file/
> directory, but not 1_files/ directory), it would fail.
> 
>> +	touch_files 10 &&
>> +	touch_files 100 &&
>> +	touch_files 1000 &&
>> +	touch_files 10000 &&
> 
> Apparently nobody has run this perf script recently since part #2
> was posted.
> 
>>   	git add 1_file 10_files 100_files 1000_files 10000_files &&
> 
> The original introduced at bb7cc7e7 (t/perf/fsmonitor: separate one
> time repo initialization, 2020-10-26) created an empty directory 1_file
> and without creating anything in it, ran "git add" on it.
> 
> If we are not doing anything to 1_file directory anyway, perhaps we
> can get rid of it to avoid the breakage in "make perf"?
> 
> If we have a chance to reroll this series, we can squash in
> something like this, perhaps (it does not deserve to be a separate
> step).

Good catch!

It looks to me like there was an oversight/typo in the original
89afd5f5ad (t/perf: add fsmonitor perf test for git diff, 2020-10-20).
They created the "1_file" directory and didn't put anything in it.
Then later when they test it, they say "0_files" in the test name
and "1_file" in the "git diff" command.

I don't think it's worth keeping an empty directory here, since there
won't be anything in the index after the add and since the directory
is empty the untracked cache won't have anything to scan.

My version (without the typo) would have created 1 file in the directory
but I don't think that's worth keeping either, since we create thousands
of files in steps right after it.

I'll make a note to remove it.

Jeff



> 
> --- >8 ---
> Subject: [PATCH] p7519: leave 1_file directory empty
> 
> The step "t/perf/p7519: speed up test on Windows" in the topic
> builtin-fsmonitor-part-2 (not in 'next' yet) attempts to create one
> file in 1_files directory, but the original introduced at bb7cc7e7
> (t/perf/fsmonitor: separate one time repo initialization,
> 2020-10-26):
> 
>   (1) created 1_file directory,
> 
>   (2) left the directory empty, and
> 
>   (3) a later test expected (and still expects) that there is nothing
>       in the directory.
> 
> Revert the behaviour back to what the original wanted to do.
> 
> Signed-off-by: Junio C Hamano <[e-mail]>
> ---
>   t/perf/p7519-fsmonitor.sh | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/t/perf/p7519-fsmonitor.sh b/t/perf/p7519-fsmonitor.sh
> index 9a2288a622..a1c552129c 100755
> --- a/t/perf/p7519-fsmonitor.sh
> +++ b/t/perf/p7519-fsmonitor.sh
> @@ -126,7 +126,7 @@ test_expect_success "one time repo setup" '
>   	fi &&
>   
>   	mkdir 1_file 10_files 100_files 1000_files 10000_files &&
> -	touch_files 1 &&
> +	: 1_file directory should be left empty &&
>   	touch_files 10 &&
>   	touch_files 100 &&
>   	touch_files 1000 &&
>

Patch

diff --git a/t/perf/p7519-fsmonitor.sh b/t/perf/p7519-fsmonitor.sh
index c8be58f3c76..054fc8d5d1d 100755
--- a/t/perf/p7519-fsmonitor.sh
+++ b/t/perf/p7519-fsmonitor.sh
@@ -72,7 +72,7 @@  then
 	fi
 fi
 
-trace_start() {
+trace_start () {
 	if test -n "$GIT_PERF_7519_TRACE"
 	then
 		name="$1"
@@ -91,13 +91,20 @@  trace_start() {
 	fi
 }
 
-trace_stop() {
+trace_stop () {
 	if test -n "$GIT_PERF_7519_TRACE"
 	then
 		unset GIT_TRACE2_PERF
 	fi
 }
 
+touch_files () {
+	n=$1
+	d="$n"_files
+
+	(cd $d ; test_seq 1 $n | xargs touch )
+}
+
 test_expect_success "one time repo setup" '
 	# set untrackedCache depending on the environment
 	if test -n "$GIT_PERF_7519_UNTRACKED_CACHE"
@@ -119,10 +126,11 @@  test_expect_success "one time repo setup" '
 	fi &&
 
 	mkdir 1_file 10_files 100_files 1000_files 10000_files &&
-	for i in $(test_seq 1 10); do touch 10_files/$i || return 1; done &&
-	for i in $(test_seq 1 100); do touch 100_files/$i || return 1; done &&
-	for i in $(test_seq 1 1000); do touch 1000_files/$i || return 1; done &&
-	for i in $(test_seq 1 10000); do touch 10000_files/$i || return 1; done &&
+	touch_files 1 &&
+	touch_files 10 &&
+	touch_files 100 &&
+	touch_files 1000 &&
+	touch_files 10000 &&
 	git add 1_file 10_files 100_files 1000_files 10000_files &&
 	git commit -qm "Add files" &&
 
@@ -133,7 +141,7 @@  test_expect_success "one time repo setup" '
 	fi
 '
 
-setup_for_fsmonitor() {
+setup_for_fsmonitor () {
 	# set INTEGRATION_SCRIPT depending on the environment
 	if test -n "$INTEGRATION_PATH"
 	then
@@ -173,7 +181,7 @@  test_perf_w_drop_caches () {
 	test_perf "$@"
 }
 
-test_fsmonitor_suite() {
+test_fsmonitor_suite () {
 	if test -n "$INTEGRATION_SCRIPT"; then
 		DESC="fsmonitor=$(basename $INTEGRATION_SCRIPT)"
 	else
@@ -199,15 +207,15 @@  test_fsmonitor_suite() {
 
 	# Update the mtimes on upto 100k files to make status think
 	# that they are dirty.  For simplicity, omit any files with
-	# LFs (i.e. anything that ls-files thinks it needs to dquote).
-	# Then fully backslash-quote the paths to capture any
-	# whitespace so that they pass thru xargs properly.
+	# LFs (i.e. anything that ls-files thinks it needs to dquote)
+	# and any files with whitespace so that they pass thru xargs
+	# properly.
 	#
 	test_perf_w_drop_caches "status (dirty) ($DESC)" '
 		git ls-files | \
 			head -100000 | \
 			grep -v \" | \
-			sed '\''s/\(.\)/\\\1/g'\'' | \
+			egrep -v " ." | \
 			xargs test-tool chmtime -300 &&
 		git status
 	'