Re: [PATCH v4 00/13] core.fsyncmethod: add 'batch' mode for faster fsyncing of multiple objects

From: Neeraj Singh <nksingh85@gmail.com>
Date: Mar 29, 2022, 17:09 UTC
Message-ID: <CANQDOdf2tXM6f2U5=68Htw+AGyrAM0C6UAVPmfxCk8Ga7pek2g@mail.gmail.com>
In-Reply-To: <220329.86czi52ekn.gmgdl@evledraar.gmail.com>

On Tue, Mar 29, 2022 at 4:17 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 28 quoted lines

>
>
> On Tue, Mar 29 2022, Neeraj K. Singh via GitGitGadget wrote:
>
> > V4 changes:
> >
> >  * Make ODB transactions nestable.
> >  * Add an ODB transaction around writing out the cached tree.
> >  * Change update-index to use a more straightforward way of managing ODB
> >    transactions.
> >  * Fix missing 'local's in lib-unique-files
> >  * Add a per-iteration setup mechanism to test_perf.
> >  * Fix camelCasing in warning message.
>
> I haven't looked at the bulk of this in any detail, but:
>
> >  10:  b99b32a469c ! 12:  fdf90d45f52 core.fsyncmethod: performance tests for add and stash
> >      @@ t/perf/p3700-add.sh (new)
> >       +# core.fsyncMethod=batch mode, which is why we are testing different values
> >       +# of that setting explicitly and creating a lot of unique objects.
> >       +
> >      -+test_description="Tests performance of add"
> >      ++test_description="Tests performance of adding things to the object database"
>
> Now having both tests for "add" and "stash" in a test named p3700-add.sh
> isn't better, the rest of the perf tests are split up by command,
> perhaps just add a helper library and have both use it?
>

I was getting tired of editing two files that were nearly identical and thought that reviewers would be tired of reading them. At least in the main test suite, t/t3700-add.sh covers update-index in addition to git-add.

Show 5 quoted lines

> And re the unaddressed feedback I ad of "why the random data"
> inhttps://lore.kernel.org/git/220326.86o81sk9ao.gmgdl@evledraar.gmail.com/
> I tried patching it on top to do what I suggested there, allowing us to
> run these against any arbitrary repository and came up with this:
>

The advantage of the random data is that it's easy to scale the number of files and change the tree shape. I wanted the test_create_unique_files helper anyway, so using it here made sense. Also, I'm quite confident that I'm really getting new objects added to the repo with this test scheme.

Show 201 quoted lines

> diff --git a/t/perf/p3700-add.sh b/t/perf/p3700-add.sh
> index ef6024f9897..60abd5ee076 100755
> --- a/t/perf/p3700-add.sh
> +++ b/t/perf/p3700-add.sh
> @@ -13,47 +13,26 @@ export GIT_TEST_FSYNC
>
>  . ./perf-lib.sh
>
> -. $TEST_DIRECTORY/lib-unique-files.sh
> -
> -test_perf_fresh_repo
> +test_perf_default_repo
>  test_checkout_worktree
>
> -dir_count=10
> -files_per_dir=50
> -total_files=$((dir_count * files_per_dir))
> -
> -for mode in false true batch
> +for cfg in \
> +       '-c core.fsync=-loose-object -c core.fsyncmethod=fsync' \
> +       '-c core.fsync=loose-object -c core.fsyncmethod=fsync' \
> +       '-c core.fsync=loose-object -c core.fsyncmethod=batch' \
> +       '-c core.fsyncmethod=batch'
>  do
> -       case $mode in
> -       false)
> -               FSYNC_CONFIG='-c core.fsync=-loose-object -c core.fsyncmethod=fsync'
> -               ;;
> -       true)
> -               FSYNC_CONFIG='-c core.fsync=loose-object -c core.fsyncmethod=fsync'
> -               ;;
> -       batch)
> -               FSYNC_CONFIG='-c core.fsync=loose-object -c core.fsyncmethod=batch'
> -               ;;
> -       esac
> -
> -       test_perf "add $total_files files (object_fsyncing=$mode)" \
> -               --setup "
> -               (rm -rf .git || 1) &&
> -               git init &&
> -               test_create_unique_files $dir_count $files_per_dir files_$mode
> -       " "
> -               git $FSYNC_CONFIG add files_$mode
> -       "
> -
> -       test_perf "stash $total_files files (object_fsyncing=$mode)" \
> -               --setup "
> -               (rm -rf .git || 1) &&
> -               git init &&
> -               test_commit first &&
> -               test_create_unique_files $dir_count $files_per_dir stash_files_$mode
> -       " "
> -               git $FSYNC_CONFIG stash push -u -- stash_files_$mode
> -       "
> +       test_perf "'git add' with '$cfg'" \
> +               --setup '
> +                       mv -v .git .git.old &&
> +                       git init .
> +               ' \
> +               --cleanup '
> +                       rm -rf .git &&
> +                       mv .git.old .git
> +               ' '
> +               git $cfg add -f -- ":!.git.old/"
> +       '
>  done
>
>  test_done
> diff --git a/t/perf/p3900-stash.sh b/t/perf/p3900-stash.sh
> new file mode 100755
> index 00000000000..12c489069ba
> --- /dev/null
> +++ b/t/perf/p3900-stash.sh
> @@ -0,0 +1,34 @@
> +#!/bin/sh
> +
> +test_description='performance of "git stash" with different fsync settings'
> +
> +# Fsync is normally turned off for the test suite.
> +GIT_TEST_FSYNC=1
> +export GIT_TEST_FSYNC
> +
> +. ./perf-lib.sh
> +
> +test_perf_default_repo
> +test_checkout_worktree
> +
> +for cfg in \
> +       '-c core.fsync=-loose-object -c core.fsyncmethod=fsync' \
> +       '-c core.fsync=loose-object -c core.fsyncmethod=fsync' \
> +       '-c core.fsync=loose-object -c core.fsyncmethod=batch' \
> +       '-c core.fsyncmethod=batch'
> +do
> +       test_perf "'stash push -u' with '$cfg'" \
> +               --setup '
> +                       mv -v .git .git.old &&
> +                       git init . &&
> +                       test_commit dummy
> +               ' \
> +               --cleanup '
> +                       rm -rf .git &&
> +                       mv .git.old .git
> +               ' '
> +               git $cfg stash push -a -u ":!.git.old/" ":!test*" "."
> +       '
> +done
> +
> +test_done
> diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
> index a935ad622d3..24a5108f234 100644
> --- a/t/perf/perf-lib.sh
> +++ b/t/perf/perf-lib.sh
> @@ -194,6 +194,7 @@ test_wrapper_ () {
>         test_start_
>         test_prereq=
>         test_perf_setup_=
> +       test_perf_cleanup_=
>         while test $# != 0
>         do
>                 case $1 in
> @@ -205,6 +206,10 @@ test_wrapper_ () {
>                         test_perf_setup_=$2
>                         shift
>                         ;;
> +               --cleanup)
> +                       test_perf_cleanup_=$2
> +                       shift
> +                       ;;
>                 *)
>                         break
>                         ;;
> @@ -214,6 +219,7 @@ test_wrapper_ () {
>         test "$#" = 1 || BUG "test_wrapper_ needs 2 positional parameters"
>         export test_prereq
>         export test_perf_setup_
> +       export test_perf_cleanup_
>         if ! test_skip "$test_title_" "$@"
>         then
>                 base=$(basename "$0" .sh)
> @@ -256,6 +262,16 @@ test_perf_ () {
>                         test_failure_ "$@"
>                         break
>                 fi
> +               if test -n "$test_perf_cleanup_"
> +               then
> +                       say >&3 "cleanup: $test_perf_cleanup_"
> +                       if ! test_eval_ $test_perf_cleanup_
> +                       then
> +                               test_failure_ "$test_perf_cleanup_"
> +                               break
> +                       fi
> +
> +               fi
>         done
>         if test -z "$verbose"; then
>                 echo " ok"
>
>
> Here it is against Cor.git (a random small-ish repo I had laying around):
>
>         $ GIT_SKIP_TESTS='p3[79]00.[12]' GIT_PERF_MAKE_OPTS='CFLAGS=-O3' GIT_PERF_REPO=~/g/Cor/ ./run origin/master HEAD -- p3900-stash.sh
>         === Building abf474a5dd901f28013c52155411a48fd4c09922 (origin/master) ===
>             GEN git-add--interactive
>             GEN git-archimport
>             GEN git-cvsexportcommit
>             GEN git-cvsimport
>             GEN git-cvsserver
>             GEN git-send-email
>             GEN git-svn
>             GEN git-p4
>             SUBDIR templates
>         === Running 1 tests in /home/avar/g/git/t/perf/build/abf474a5dd901f28013c52155411a48fd4c09922/bin-wrappers ===
>         ok 1 # skip 'stash push -u' with '-c core.fsync=-loose-object -c core.fsyncmethod=fsync' (GIT_SKIP_TESTS)
>         ok 2 # skip 'stash push -u' with '-c core.fsync=loose-object -c core.fsyncmethod=fsync' (GIT_SKIP_TESTS)
>         perf 3 - 'stash push -u' with '-c core.fsync=loose-object -c core.fsyncmethod=batch': 1 2 3 ok
>         perf 4 - 'stash push -u' with '-c core.fsyncmethod=batch': 1 2 3 ok
>         # passed all 4 test(s)
>         1..4
>         === Building ecda9c2b029e35d239e369b875b245f45fd2a097 (HEAD) ===
>             GEN git-add--interactive
>             GEN git-archimport
>             GEN git-cvsexportcommit
>             GEN git-cvsimport
>             GEN git-cvsserver
>             GEN git-send-email
>             GEN git-svn
>             GEN git-p4
>             SUBDIR templates
>         === Running 1 tests in /home/avar/g/git/t/perf/build/ecda9c2b029e35d239e369b875b245f45fd2a097/bin-wrappers ===
>         ok 1 # skip 'stash push -u' with '-c core.fsync=-loose-object -c core.fsyncmethod=fsync' (GIT_SKIP_TESTS)
>         ok 2 # skip 'stash push -u' with '-c core.fsync=loose-object -c core.fsyncmethod=fsync' (GIT_SKIP_TESTS)
>         perf 3 - 'stash push -u' with '-c core.fsync=loose-object -c core.fsyncmethod=batch': 1 2 3 ok
>         perf 4 - 'stash push -u' with '-c core.fsyncmethod=batch': 1 2 3 ok
>         # passed all 4 test(s)
>         1..4
>         Test       origin/master     HEAD
>         ---------------------------------------------------
>         3900.3:    0.03(0.00+0.00)   0.02(0.00+0.00) -33.3%
>         3900.4:    0.02(0.00+0.00)   0.03(0.00+0.00) +50.0%
>

Something is wrong with your data here. Or your repo is too small to highlight the differences.

I'd suggest that if you want to write a different perf test for this feature, that it be a follow-on change.

Thanks, Neeraj