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

[Thomas Gummerer <t.gummerer@gmail.com>]

On 04/12, erik elfström wrote:
> On Sat, Apr 11, 2015 at 7:59 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
> > On 04/11, Erik Elfström wrote:
> >> Signed-off-by: Erik Elfström <erik.elfstrom@gmail.com>
> >> ---
> >>  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.

> >> +     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