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

[Neeraj Singh <nksingh85@gmail.com>]

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

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

> 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