[PATCH] perf: accommodate for MacOSX

[PATCH] perf: accommodate for MacOSX

[Johannes Schindelin]

As this developer has no access to MacOSX developer setups anymore,
Travis becomes the best bet to run performance tests on that OS.

However, on MacOSX /usr/bin/time is that good old BSD executable that
no Linux user cares about, as demonstrated by the perf-lib.sh's use
of GNU-ish extensions. And by the hard-coded path.

Let's just work around this issue by using gtime on MacOSX, the
Homebrew-provided GNU implementation onto which pretty much every
MacOSX power user falls back anyway.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Published-As: https://github.com/dscho/git/releases/tag/perf-macosx-v1

	This is another of those patches that came up in my rebase--helper
	work, even if it is not quite obvious from the patch ;-)

 .travis.yml        | 2 +-
 t/perf/perf-lib.sh | 6 +++++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/.travis.yml b/.travis.yml
index c20ec54..0e569bc 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -82,7 +82,7 @@ before_install:
       brew tap homebrew/binary --quiet
       brew_force_set_latest_binary_hash perforce
       brew_force_set_latest_binary_hash perforce-server
-      brew install git-lfs perforce-server perforce gettext
+      brew install git-lfs perforce-server perforce gettext gnu-time
       brew link --force gettext
       ;;
     esac;
diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
index 18c363e..773f955 100644
--- a/t/perf/perf-lib.sh
+++ b/t/perf/perf-lib.sh
@@ -127,11 +127,15 @@ test_checkout_worktree () {
 # Performance tests should never fail.  If they do, stop immediately
 immediate=t
 
+# Perf tests require GNU time
+case "$(uname -s)" in Darwin) GTIME="${GTIME:-gtime}";; esac
+GTIME="${GTIME:-/usr/bin/time}"
+
 test_run_perf_ () {
 	test_cleanup=:
 	test_export_="test_cleanup"
 	export test_cleanup test_export_
-	/usr/bin/time -f "%E %U %S" -o test_time.$i "$SHELL" -c '
+	"$GTIME" -f "%E %U %S" -o test_time.$i "$SHELL" -c '
 . '"$TEST_DIRECTORY"/test-lib-functions.sh'
 test_export () {
 	[ $# != 0 ] || return 0
-- 
2.9.0.119.gb7b8d21

base-commit: 05219a1276341e72d8082d76b7f5ed394b7437a4

Re: [PATCH] perf: accommodate for MacOSX

[Lars Schneider]

> On 18 Jun 2016, at 15:03, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:
> 
> As this developer has no access to MacOSX developer setups anymore,
> Travis becomes the best bet to run performance tests on that OS.
We don't run the performance tests on Travis CI right now.
Maybe we should? With your patch below it should work, right?

I only saw one error on my local OS X machine here:
https://github.com/git/git/blob/05219a1276341e72d8082d76b7f5ed394b7437a4/t/perf/p0000-perf-lib-sanity.sh#L26

Does the export of foo not work properly on OS X? "$foo" is empty...

- Lars


> However, on MacOSX /usr/bin/time is that good old BSD executable that
> no Linux user cares about, as demonstrated by the perf-lib.sh's use
> of GNU-ish extensions. And by the hard-coded path.
> 
> Let's just work around this issue by using gtime on MacOSX, the
> Homebrew-provided GNU implementation onto which pretty much every
> MacOSX power user falls back anyway.
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> Published-As: https://github.com/dscho/git/releases/tag/perf-macosx-v1
> 
> 	This is another of those patches that came up in my rebase--helper
> 	work, even if it is not quite obvious from the patch ;-)
> 
> .travis.yml        | 2 +-
> t/perf/perf-lib.sh | 6 +++++-
> 2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/.travis.yml b/.travis.yml
> index c20ec54..0e569bc 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -82,7 +82,7 @@ before_install:
>       brew tap homebrew/binary --quiet
>       brew_force_set_latest_binary_hash perforce
>       brew_force_set_latest_binary_hash perforce-server
> -      brew install git-lfs perforce-server perforce gettext
> +      brew install git-lfs perforce-server perforce gettext gnu-time
>       brew link --force gettext
>       ;;
>     esac;
> diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
> index 18c363e..773f955 100644
> --- a/t/perf/perf-lib.sh
> +++ b/t/perf/perf-lib.sh
> @@ -127,11 +127,15 @@ test_checkout_worktree () {
> # Performance tests should never fail.  If they do, stop immediately
> immediate=t
> 
> +# Perf tests require GNU time
> +case "$(uname -s)" in Darwin) GTIME="${GTIME:-gtime}";; esac
> +GTIME="${GTIME:-/usr/bin/time}"
> +
> test_run_perf_ () {
> 	test_cleanup=:
> 	test_export_="test_cleanup"
> 	export test_cleanup test_export_
> -	/usr/bin/time -f "%E %U %S" -o test_time.$i "$SHELL" -c '
> +	"$GTIME" -f "%E %U %S" -o test_time.$i "$SHELL" -c '
> . '"$TEST_DIRECTORY"/test-lib-functions.sh'
> test_export () {
> 	[ $# != 0 ] || return 0
> -- 
> 2.9.0.119.gb7b8d21
> 
> base-commit: 05219a1276341e72d8082d76b7f5ed394b7437a4
> --
> 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

Re: [PATCH] perf: accommodate for MacOSX

[Johannes Schindelin]

Hi Lars,

On Sun, 19 Jun 2016, Lars Schneider wrote:

> > On 18 Jun 2016, at 15:03, Johannes Schindelin
> > <johannes.schindelin@gmx.de> wrote:
> > 
> > As this developer has no access to MacOSX developer setups anymore,
> > Travis becomes the best bet to run performance tests on that OS.
>
> We don't run the performance tests on Travis CI right now.
> Maybe we should? With your patch below it should work, right?

It *should* work, but I'd be reluctant to run them as part of the CI: we
have no real chance to catch perf regressions as of now. So all it would
do would be to add load to Travis.

> I only saw one error on my local OS X machine here:
> https://github.com/git/git/blob/05219a1276341e72d8082d76b7f5ed394b7437a4/t/perf/p0000-perf-lib-sanity.sh#L26

Yeah, well, I should have been clearer in my commit message: this patch
allows the perf tests to *run*, not to *pass*... ;-)

> Does the export of foo not work properly on OS X? "$foo" is empty...

Sorry, as I said: I no longer have access to a dev setup running MacOSX.

Ciao,
Dscho

P.S.: Please save me time by deleting the remainder of the quoted mail
when it is irrelevant to your reply. Thanks!

Re: [PATCH] perf: accommodate for MacOSX

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Sun, 19 Jun 2016, Lars Schneider wrote:
>
>> > On 18 Jun 2016, at 15:03, Johannes Schindelin
>> > <johannes.schindelin@gmx.de> wrote:
>> > 
>> > As this developer has no access to MacOSX developer setups anymore,
>> > Travis becomes the best bet to run performance tests on that OS.
>>
>> We don't run the performance tests on Travis CI right now.
>> Maybe we should? With your patch below it should work, right?
> ...
>
> Yeah, well, I should have been clearer in my commit message: this patch
> allows the perf tests to *run*, not to *pass*... ;-)

OK, Lars, do we still want to take this patch?  I am leaning towards
taking it, if only to motivate interested others with OS X to look
into making the perf tests to actually run.

Re: [PATCH] perf: accommodate for MacOSX

[Lars Schneider]

> On 20 Jun 2016, at 21:48, Junio C Hamano <gitster@pobox.com> wrote:
> 
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
>> On Sun, 19 Jun 2016, Lars Schneider wrote:
>> 
>>>> On 18 Jun 2016, at 15:03, Johannes Schindelin
>>>> <johannes.schindelin@gmx.de> wrote:
>>>> 
>>>> As this developer has no access to MacOSX developer setups anymore,
>>>> Travis becomes the best bet to run performance tests on that OS.
>>> 
>>> We don't run the performance tests on Travis CI right now.
>>> Maybe we should? With your patch below it should work, right?
>> ...
>> 
>> Yeah, well, I should have been clearer in my commit message: this patch
>> allows the perf tests to *run*, not to *pass*... ;-)
> 
> OK, Lars, do we still want to take this patch?  I am leaning towards
> taking it, if only to motivate interested others with OS X to look
> into making the perf tests to actually run.

I think we definitively should take the "perf-lib.sh" part of the patch
as this makes the perf test run on OSX and therefore is a strict
improvement.

If we don't run any perf tests by default on Travis CI then I wouldn't 
take the ".travis.yml" part of the patch just to keep our Travis CI
setup as lean as possible. Running perf tests on Travis CI is probably
bogus anyways because we never know on what hardware our jobs run
and what other jobs run in parallel on that hardware.

- Lars

Re: [PATCH] perf: accommodate for MacOSX

[Johannes Schindelin]

Hi Lars,

On Tue, 21 Jun 2016, Lars Schneider wrote:

> > On 20 Jun 2016, at 21:48, Junio C Hamano <gitster@pobox.com> wrote:
> > 
> > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> > 
> >> On Sun, 19 Jun 2016, Lars Schneider wrote:
> >> 
> >>>> On 18 Jun 2016, at 15:03, Johannes Schindelin
> >>>> <johannes.schindelin@gmx.de> wrote:
> >>>> 
> >>>> As this developer has no access to MacOSX developer setups anymore,
> >>>> Travis becomes the best bet to run performance tests on that OS.
> >>> 
> >>> We don't run the performance tests on Travis CI right now.
> >>> Maybe we should? With your patch below it should work, right?
> >> ...
> >> 
> >> Yeah, well, I should have been clearer in my commit message: this patch
> >> allows the perf tests to *run*, not to *pass*... ;-)
> > 
> > OK, Lars, do we still want to take this patch?  I am leaning towards
> > taking it, if only to motivate interested others with OS X to look
> > into making the perf tests to actually run.
> 
> I think we definitively should take the "perf-lib.sh" part of the patch
> as this makes the perf test run on OSX and therefore is a strict
> improvement.

Yes, it was meant as the starting point to get more things to run on
MacOSX.

> If we don't run any perf tests by default on Travis CI then I wouldn't
> take the ".travis.yml" part of the patch just to keep our Travis CI
> setup as lean as possible.

Maybe commented-out, so that people like me have a chance to use Travis
for MacOSX perf testing?

> Running perf tests on Travis CI is probably bogus anyways because we
> never know on what hardware our jobs run and what other jobs run in
> parallel on that hardware.

While I agree that the absolute timings cannot be trusted, I have to point
out that the relative timings on Linux at least are consistent with what I
could test locally.

Could you let me know whether a commented-out

	# Uncomment this if you want to run perf tests:
	# brew install gnu-time

would be acceptable? I will reroll the patch accordingly.

Ciao,
Dscho

Re: [PATCH] perf: accommodate for MacOSX

[Lars Schneider]

> On 21 Jun 2016, at 13:55, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> 
>> ...
>> I think we definitively should take the "perf-lib.sh" part of the patch
>> as this makes the perf test run on OSX and therefore is a strict
>> improvement.
> 
> Yes, it was meant as the starting point to get more things to run on
> MacOSX.
> 
>> If we don't run any perf tests by default on Travis CI then I wouldn't
>> take the ".travis.yml" part of the patch just to keep our Travis CI
>> setup as lean as possible.
> 
> Maybe commented-out, so that people like me have a chance to use Travis
> for MacOSX perf testing?
> 
>> Running perf tests on Travis CI is probably bogus anyways because we
>> never know on what hardware our jobs run and what other jobs run in
>> parallel on that hardware.
> 
> While I agree that the absolute timings cannot be trusted, I have to point
> out that the relative timings on Linux at least are consistent with what I
> could test locally.
> 
> Could you let me know whether a commented-out
> 
> 	# Uncomment this if you want to run perf tests:
> 	# brew install gnu-time
> 
> would be acceptable? I will reroll the patch accordingly.

Commented-out would be fine with me!

Independent of your patch:
Given that the relative timings are consistent for you. Maybe there is
value to run the performance tests (e.g. only on the master branch)
in a separate Travis job. Then we could chart the timings over releases.
I dunno.

- Lars

Re: [PATCH] perf: accommodate for MacOSX

[Johannes Schindelin]

Hi Lars,

On Tue, 21 Jun 2016, Lars Schneider wrote:

> > On 21 Jun 2016, at 13:55, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > 
> >> ...
> >> If we don't run any perf tests by default on Travis CI then I wouldn't
> >> take the ".travis.yml" part of the patch just to keep our Travis CI
> >> setup as lean as possible.
> > 
> > Maybe commented-out, so that people like me have a chance to use Travis
> > for MacOSX perf testing?
> > 
> > Could you let me know whether a commented-out
> > 
> > 	# Uncomment this if you want to run perf tests:
> > 	# brew install gnu-time
> > 
> > would be acceptable? I will reroll the patch accordingly.
> 
> Commented-out would be fine with me!

Okay! Updated patch coming in a moment.

> >> Running perf tests on Travis CI is probably bogus anyways because we
> >> never know on what hardware our jobs run and what other jobs run in
> >> parallel on that hardware.
> > 
> > While I agree that the absolute timings cannot be trusted, I have to point
> > out that the relative timings on Linux at least are consistent with what I
> > could test locally.
>
> Given that the relative timings are consistent for you. Maybe there is
> value to run the performance tests (e.g. only on the master branch)
> in a separate Travis job. Then we could chart the timings over releases.
> I dunno.

Sure, that would be a fine addition, if there is a way, somehow, to chart
the timings (keep in mind that Travis runs for each and every iteration of
each and every PR, in addition to the branch updates).

I do have enough on my plate, though, so I will have to hope that somebody
else picks this up.

Ciao,
Dscho

[PATCH v2] perf: accommodate for MacOSX

[Johannes Schindelin]

As this developer has no access to MacOSX developer setups anymore,
Travis becomes the best bet to run performance tests on that OS.

However, on MacOSX /usr/bin/time is that good old BSD executable that
no Linux user cares about, as demonstrated by the perf-lib.sh's use
of GNU-ish extensions. And by the hard-coded path.

Let's just work around this issue by using gtime on MacOSX, the
Homebrew-provided GNU implementation onto which pretty much every
MacOSX power user falls back anyway.

To help other developers use Travis to run performance tests on
MacOSX, the .travis.yml file now sports a commented-out line that
installs GNU time via Homebrew.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Published-As: https://github.com/dscho/git/releases/tag/perf-macosx-v2
 .travis.yml        | 2 ++
 t/perf/perf-lib.sh | 6 +++++-
 2 files changed, 7 insertions(+), 1 deletion(-)
Interdiff vs v1:

 diff --git a/.travis.yml b/.travis.yml
 index 0e569bc..c2b76f9 100644
 --- a/.travis.yml
 +++ b/.travis.yml
 @@ -82,7 +82,9 @@ before_install:
        brew tap homebrew/binary --quiet
        brew_force_set_latest_binary_hash perforce
        brew_force_set_latest_binary_hash perforce-server
 -      brew install git-lfs perforce-server perforce gettext gnu-time
 +      # Uncomment this if you want to run perf tests:
 +      # brew install gnu-time
 +      brew install git-lfs perforce-server perforce gettext
        brew link --force gettext
        ;;
      esac;


diff --git a/.travis.yml b/.travis.yml
index c20ec54..c2b76f9 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -82,6 +82,8 @@ before_install:
       brew tap homebrew/binary --quiet
       brew_force_set_latest_binary_hash perforce
       brew_force_set_latest_binary_hash perforce-server
+      # Uncomment this if you want to run perf tests:
+      # brew install gnu-time
       brew install git-lfs perforce-server perforce gettext
       brew link --force gettext
       ;;
diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
index 18c363e..773f955 100644
--- a/t/perf/perf-lib.sh
+++ b/t/perf/perf-lib.sh
@@ -127,11 +127,15 @@ test_checkout_worktree () {
 # Performance tests should never fail.  If they do, stop immediately
 immediate=t
 
+# Perf tests require GNU time
+case "$(uname -s)" in Darwin) GTIME="${GTIME:-gtime}";; esac
+GTIME="${GTIME:-/usr/bin/time}"
+
 test_run_perf_ () {
 	test_cleanup=:
 	test_export_="test_cleanup"
 	export test_cleanup test_export_
-	/usr/bin/time -f "%E %U %S" -o test_time.$i "$SHELL" -c '
+	"$GTIME" -f "%E %U %S" -o test_time.$i "$SHELL" -c '
 . '"$TEST_DIRECTORY"/test-lib-functions.sh'
 test_export () {
 	[ $# != 0 ] || return 0
-- 
2.9.0.118.g0e1a633

base-commit: ab7797dbe95fff38d9265869ea367020046db118

Re: [PATCH v2] perf: accommodate for MacOSX

[Lars Schneider]

> On 21 Jun 2016, at 15:53, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:
> 
> As this developer has no access to MacOSX developer setups anymore,
> Travis becomes the best bet to run performance tests on that OS.
> 
> However, on MacOSX /usr/bin/time is that good old BSD executable that
> no Linux user cares about, as demonstrated by the perf-lib.sh's use
> of GNU-ish extensions. And by the hard-coded path.
> 
> Let's just work around this issue by using gtime on MacOSX, the
> Homebrew-provided GNU implementation onto which pretty much every
> MacOSX power user falls back anyway.
> 
> To help other developers use Travis to run performance tests on
> MacOSX, the .travis.yml file now sports a commented-out line that
> installs GNU time via Homebrew.
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> Published-As: https://github.com/dscho/git/releases/tag/perf-macosx-v2
> .travis.yml        | 2 ++
> t/perf/perf-lib.sh | 6 +++++-
> 2 files changed, 7 insertions(+), 1 deletion(-)
> Interdiff vs v1:
> 
> diff --git a/.travis.yml b/.travis.yml
> index 0e569bc..c2b76f9 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -82,7 +82,9 @@ before_install:
>        brew tap homebrew/binary --quiet
>        brew_force_set_latest_binary_hash perforce
>        brew_force_set_latest_binary_hash perforce-server
> -      brew install git-lfs perforce-server perforce gettext gnu-time
> +      # Uncomment this if you want to run perf tests:
> +      # brew install gnu-time
> +      brew install git-lfs perforce-server perforce gettext
>        brew link --force gettext
>        ;;
>      esac;
> 
> 
> diff --git a/.travis.yml b/.travis.yml
> index c20ec54..c2b76f9 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -82,6 +82,8 @@ before_install:
>       brew tap homebrew/binary --quiet
>       brew_force_set_latest_binary_hash perforce
>       brew_force_set_latest_binary_hash perforce-server
> +      # Uncomment this if you want to run perf tests:
> +      # brew install gnu-time
>       brew install git-lfs perforce-server perforce gettext
>       brew link --force gettext
>       ;;
> diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
> index 18c363e..773f955 100644
> --- a/t/perf/perf-lib.sh
> +++ b/t/perf/perf-lib.sh
> @@ -127,11 +127,15 @@ test_checkout_worktree () {
> # Performance tests should never fail.  If they do, stop immediately
> immediate=t
> 
> +# Perf tests require GNU time
> +case "$(uname -s)" in Darwin) GTIME="${GTIME:-gtime}";; esac
> +GTIME="${GTIME:-/usr/bin/time}"
> +
> test_run_perf_ () {
> 	test_cleanup=:
> 	test_export_="test_cleanup"
> 	export test_cleanup test_export_
> -	/usr/bin/time -f "%E %U %S" -o test_time.$i "$SHELL" -c '
> +	"$GTIME" -f "%E %U %S" -o test_time.$i "$SHELL" -c '
> . '"$TEST_DIRECTORY"/test-lib-functions.sh'
> test_export () {
> 	[ $# != 0 ] || return 0
> -- 
> 2.9.0.118.g0e1a633
> 
> base-commit: ab7797dbe95fff38d9265869ea367020046db118

Looks good to me.

- Lars

Re: [PATCH v2] perf: accommodate for MacOSX

[Junio C Hamano]

Lars Schneider <larsxschneider@gmail.com> writes:

>> On 21 Jun 2016, at 15:53, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:
>> 
> Looks good to me.

Thanks, both, for following thru.
Will apply.