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

From: erik elfström <erik.elfstrom@gmail.com>
Date: Apr 12, 2015, 15:31 UTC
Message-ID: <CAMpP7NZyHBMZ5omT+fPGfkRR56HQPNP3n6cXD7S8voVvqrG6eg@mail.gmail.com>
In-Reply-To: <20150411175941.GA32735@hank>

On Sat, Apr 11, 2015 at 7:59 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote:

Show 46 quoted lines

> 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).

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?

Show 19 quoted lines

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