| Message ID | 42ab39f21212d3da1af3546d3425aa790637056f.1661961746.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | scalar: integrate into core Git | expand |
Hi Victoria, On Wed, 31 Aug 2022, Victoria Dye via GitGitGadget wrote: > From: Victoria Dye <vdye@github.com> > > Create 'p9210-scalar.sh' for testing Scalar performance and comparing > performance of Git operations in Scalar registrations and standard > repositories. Example results: > > Test this tree > ------------------------------------------------------------------------ > 9210.2: scalar clone 14.82(18.00+3.63) > 9210.3: git clone 26.15(36.67+6.90) > 9210.4: git status (scalar) 0.04(0.01+0.01) > 9210.5: git status (non-scalar) 0.10(0.02+0.11) > 9210.6: test_commit --append --no-tag A (scalar) 0.08(0.02+0.03) > 9210.7: test_commit --append --no-tag A (non-scalar) 0.13(0.03+0.11) Excellent! > [...] > + > +test_compare_perf () { > + command="$@" > + test_perf "$command (scalar)" " > + ( > + cd scalar-clone/src && > + $command > + ) > + " > + > + test_perf "$command (non-scalar)" " > + ( > + cd git-clone && > + $command > + ) > + " > +} > + > +test_compare_perf git status > +test_compare_perf test_commit --append --no-tag A Given the small numbers presented in the commit message, I suspect that even so much as running the command in a subshell might skew the timings at least on Windows, where subshells are very, very expensive. Given that both `git` and `test_commit` understand the `-C <directory>` syntax, this variant would resolve my concern: test_compare_perf () { command=$1 shift args="$*" test_perf "$command (scalar)" " $command -C scalar-clone/src $args " test_perf "$command (non-scalar)" " $command -C git-clone $args " } What do you think? Ciao, Dscho
Johannes Schindelin wrote: > Hi Victoria, > > On Wed, 31 Aug 2022, Victoria Dye via GitGitGadget wrote: > >> [...] >> + >> +test_compare_perf () { >> + command="$@" >> + test_perf "$command (scalar)" " >> + ( >> + cd scalar-clone/src && >> + $command >> + ) >> + " >> + >> + test_perf "$command (non-scalar)" " >> + ( >> + cd git-clone && >> + $command >> + ) >> + " >> +} >> + >> +test_compare_perf git status >> +test_compare_perf test_commit --append --no-tag A > > Given the small numbers presented in the commit message, I suspect that > even so much as running the command in a subshell might skew the timings > at least on Windows, where subshells are very, very expensive. > > Given that both `git` and `test_commit` understand the `-C <directory>` > syntax, this variant would resolve my concern: > > test_compare_perf () { > command=$1 > shift > args="$*" > > test_perf "$command (scalar)" " > $command -C scalar-clone/src $args > " > > test_perf "$command (non-scalar)" " > $command -C git-clone $args > " > } > > What do you think? Makes sense to me! Although, out of curiosity, is there a reason you prefer "$1 -> shift -> $*" over '$1' and '$@'? > > Ciao, > Dscho
Victoria Dye wrote: > Johannes Schindelin wrote: >> Hi Victoria, >> >> On Wed, 31 Aug 2022, Victoria Dye via GitGitGadget wrote: >> >>> [...] >>> + >>> +test_compare_perf () { >>> + command="$@" >>> + test_perf "$command (scalar)" " >>> + ( >>> + cd scalar-clone/src && >>> + $command >>> + ) >>> + " >>> + >>> + test_perf "$command (non-scalar)" " >>> + ( >>> + cd git-clone && >>> + $command >>> + ) >>> + " >>> +} >>> + >>> +test_compare_perf git status >>> +test_compare_perf test_commit --append --no-tag A >> >> Given the small numbers presented in the commit message, I suspect that >> even so much as running the command in a subshell might skew the timings >> at least on Windows, where subshells are very, very expensive. >> >> Given that both `git` and `test_commit` understand the `-C <directory>` >> syntax, this variant would resolve my concern: >> >> test_compare_perf () { >> command=$1 >> shift >> args="$*" >> >> test_perf "$command (scalar)" " >> $command -C scalar-clone/src $args >> " >> >> test_perf "$command (non-scalar)" " >> $command -C git-clone $args >> " >> } >> >> What do you think? > > Makes sense to me! Although, out of curiosity, is there a reason you prefer > "$1 -> shift -> $*" over '$1' and '$@'? Whoops, I completely misread your snippet; the 'shift' is necessary to separate the '$command' out so that we can inject '-C'. Thanks! > >> >> Ciao, >> Dscho >
"Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes: > +test_compare_perf () { > + command="$@" > + test_perf "$command (scalar)" " > + ( > + cd scalar-clone/src && > + $command > + ) > + " Our preference is to avoid using "$@" when you are not taking advantage of the fact that it protects individual parameters from getting split at $IFS whitespaces. Use "$*" is preferred here [*]. For example, this is good: mytest () { for arg in one two "$@" three do do_something_to "$arg" done } mytest 'a b c' 'd' Thanks to the use of "$@", 'a b c' stays together and are kept distinct from 'd'. The above is not. command="$@" is used as a misleading synonym for command="$*" that flattens the arguments to test_compare_perf function. If you called it with 'a b c' and 'd' like we called mytest with, you cannot tell that 'a b c' were together and 'd' was distinct from the other three. The only thing test_perf sees would be that $command without double quotes have four separate tokens. [Footnote] * Of course, it is tolerated only in tests and perfs where we are in total control of the arguments so that we can declare that no args to the shell function have whitespace in them. In scripts used by end-users, we may not be able to get away with "$*".
Hi Victoria, On Thu, 1 Sep 2022, Victoria Dye wrote: > Victoria Dye wrote: > > Johannes Schindelin wrote: > >> > >> On Wed, 31 Aug 2022, Victoria Dye via GitGitGadget wrote: > >> > >>> [...] > >>> + > >>> +test_compare_perf () { > >>> + command="$@" > >>> + test_perf "$command (scalar)" " > >>> + ( > >>> + cd scalar-clone/src && > >>> + $command > >>> + ) > >>> + " > >>> + > >>> + test_perf "$command (non-scalar)" " > >>> + ( > >>> + cd git-clone && > >>> + $command > >>> + ) > >>> + " > >>> +} > >>> + > >>> +test_compare_perf git status > >>> +test_compare_perf test_commit --append --no-tag A > >> > >> Given the small numbers presented in the commit message, I suspect that > >> even so much as running the command in a subshell might skew the timings > >> at least on Windows, where subshells are very, very expensive. > >> > >> Given that both `git` and `test_commit` understand the `-C <directory>` > >> syntax, this variant would resolve my concern: > >> > >> test_compare_perf () { > >> command=$1 > >> shift > >> args="$*" > >> > >> test_perf "$command (scalar)" " > >> $command -C scalar-clone/src $args > >> " > >> > >> test_perf "$command (non-scalar)" " > >> $command -C git-clone $args > >> " > >> } > >> > >> What do you think? > > > > Makes sense to me! Although, out of curiosity, is there a reason you prefer > > "$1 -> shift -> $*" over '$1' and '$@'? > > Whoops, I completely misread your snippet; the 'shift' is necessary to > separate the '$command' out so that we can inject '-C'. Yes, and I also changed the "$@" (which would usually expand to a parameter list, except when it is used inside a string, in which case it behaves like $* for convenience) because the "$*" conveys more correctly what we do here. Whenever I read "$@" anywhere, my mind puts a mental check mark behind the "is this code safe with regards to spaces in arguments?" question. However, this function would mishandle arguments that contain spaces, and reading "$*" makes me aware of that, so that I can avoid passing such arguments. So for me, using $* here is the right thing to do: It makes it less likely that someone like me adds code in the future that assumes that even arguments with spaces in them would be handled. Thanks, Dscho
diff --git a/t/perf/p9210-scalar.sh b/t/perf/p9210-scalar.sh new file mode 100755 index 00000000000..a68eb6b223d --- /dev/null +++ b/t/perf/p9210-scalar.sh @@ -0,0 +1,43 @@ +#!/bin/sh + +test_description='test scalar performance' +. ./perf-lib.sh + +test_perf_large_repo "$TRASH_DIRECTORY/to-clone" + +test_expect_success 'enable server-side partial clone' ' + git -C to-clone config uploadpack.allowFilter true && + git -C to-clone config uploadpack.allowAnySHA1InWant true +' + +test_perf 'scalar clone' ' + rm -rf scalar-clone && + scalar clone "file://$(pwd)/to-clone" scalar-clone +' + +test_perf 'git clone' ' + rm -rf git-clone && + git clone "file://$(pwd)/to-clone" git-clone +' + +test_compare_perf () { + command="$@" + test_perf "$command (scalar)" " + ( + cd scalar-clone/src && + $command + ) + " + + test_perf "$command (non-scalar)" " + ( + cd git-clone && + $command + ) + " +} + +test_compare_perf git status +test_compare_perf test_commit --append --no-tag A + +test_done