Re: [PATCH v2 2/3] p7300: add performance tests for clean

On 04/12, erik elfström wrote:

On Sat, Apr 11, 2015 at 7:59 PM, Thomas Gummerer [off-list ref] wrote:

quoted

On 04/11, Erik Elfström wrote:

quoted

Signed-off-by: Erik Elfström <redacted>
---
 t/perf/p7300-clean.sh | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)
 create mode 100755 t/perf/p7300-clean.sh

diff --git a/t/perf/p7300-clean.sh b/t/perf/p7300-clean.sh
new file mode 100755
index 0000000..af50d5d
--- /dev/null
+++ b/t/perf/p7300-clean.sh

@@ -0,0 +1,37 @@
+#!/bin/sh
+
+test_description="Test git-clean performance"
+
+. ./perf-lib.sh
+
+test_perf_large_repo
+test_checkout_worktree
+
+test_expect_success 'setup untracked directory with many sub dirs' '
+     rm -rf 500_sub_dirs 50000_sub_dirs clean_test_dir &&
+     mkdir 500_sub_dirs 50000_sub_dirs clean_test_dir &&
+     for i in $(test_seq 1 500)
+     do
+             mkdir 500_sub_dirs/dir$i || return $?
+     done &&
+     for i in $(test_seq 1 100)
+     do
+             cp -r 500_sub_dirs 50000_sub_dirs/dir$i || return $?
+     done
+'
+
+test_perf 'clean many untracked sub dirs, check for nested git' '
+     rm -rf clean_test_dir/50000_sub_dirs_cpy &&
+     cp -r 50000_sub_dirs clean_test_dir/50000_sub_dirs_cpy &&

Maybe this would be a good place to use test_perf_cleanup, which I
introduced a while ago and you can find in the
tg/perf-lib-test-perf-cleanup branch?  It probably won't influence the
performance a lot, but still better separate the code that actually
needs to be tested from the cleanup/preparation code.  Ditto in the
other test.

Yes, that would be a clear improvement. I was looking for something like
this, the copy takes more time than the clean currently.

The cleanup hook is maybe not exactly the right fit here though. I would
need to do one initial copy in the setup test and then a copy in the
cleanup, something like this:

test_expect_success 'setup untracked directory with many sub dirs' '
    ...
    cp -r 50000_sub_dirs clean_test_dir/50000_sub_dirs_cpy
'

test_perf_cleanup 'clean many untracked sub dirs, check for nested git' '
    git clean -q -f -d  clean_test_dir/
' '
    test_dir_is_empty clean_test_dir &&
    rm -rf clean_test_dir/50000_sub_dirs_cpy &&
    cp -r 50000_sub_dirs clean_test_dir/50000_sub_dirs_cpy
'

This works better than my original code but maybe we can do even better
with something like:

test_setup_perf_cleanup 'clean many untracked sub dirs, check for nested git' '
    rm -rf clean_test_dir/50000_sub_dirs_cpy &&
    cp -r 50000_sub_dirs clean_test_dir/50000_sub_dirs_cpy
' '
    git clean -q -f -d  clean_test_dir/
' '
    test_dir_is_empty clean_test_dir
'

Having a setup phase avoids the initial copy in the setup test making
things a little easier to follow. I'm not sure its worth the extra complexity
in perf-lib though (and I'm not sure I would be able to implement it either).

Yeah, you're right, that would look even better.  In order to avoid
the extra complexity we might actually go the other route that was
proposed by Junio in the discussion back then
(http://thread.gmane.org/gmane.comp.version-control.git/234874/focus=235241).

Using that and adding a setup option we could have something like:

test_perf --setup '
	rm -rf clean_test_dir/50000_sub_dirs_cpy &&
	cp -r 50000_sub_dirs clean_test_dir/50000_sub_dirs_cpy
' --cleanup '
	test_dir_is_empty clean_test_dir
' 'clean many untracked sub dirs, check for nested git' '
	git clean -q -f -d clean_test_dir
'

Also, what would the implications be if I were to use your new cleanup
function that is not yet on master? Should I rebase on top of your topic
or make a follow up patch to switch over?

If the consensus is to go that route, I think you could just rebase
the topic onto my topic.  If we introduce the "modern" test style as
described before it might be worth merging this series first and then
switch over to the new style.  You should probably wait for other
opinions before changing it though.

quoted

quoted

+     git clean -q -f -d  clean_test_dir/ &&
+     test_dir_is_empty clean_test_dir
+'
+
+test_perf 'clean many untracked sub dirs, ignore nested git' '
+     rm -rf clean_test_dir/50000_sub_dirs_cpy &&
+     cp -r 50000_sub_dirs clean_test_dir/50000_sub_dirs_cpy &&
+     git clean -q -f -f -d  clean_test_dir/ &&
+     test_dir_is_empty clean_test_dir
+'
+
+test_done
--
2.4.0.rc0.37.ga3b75b3

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
Thomas Gummerer