[PATCH] CI: add SANITIZE=[address|undefined] jobs

[PATCH] CI: add SANITIZE=[address|undefined] jobs

[Ævar Arnfjörð Bjarmason]

Add CI targets for SANITIZE=address and SANITIZE=undefined. The former
would have caught a regression in 18bbc795fc5 (Merge branch
'gc/bare-repo-discovery', 2022-07-22) which made its way to
"master"[1].

Per [2] the GitHub fork of git.git runs with these in CI, so it's
already useful to some forks of this repository.

Also per [2] we could use SANITIZE=address with some ASAN_OPTIONS
instead of our SANITIZE=leak job added in 956d2e4639b (tests: add a
test mode for SANITIZE=leak, run it in CI, 2021-09-23), but unifying
those two with these new jobs would be a lot harder, so let's leave
that for now.

On my system a "make test" takes around 12m of user time,
SANITIZE=address around 44m, and SANITIZE=undefined around 18m.

In practice this doesn't seem to slow down the wallclock time of the
the GitHub CI by much This runs in about 50m, but getting through some
of the "win build" and associated tests can take around 40m, and the
"OSX" tests are on the order of 40m (all of this may vary with the
available workers etc.).

The "address" job will fail due to the issue reported in [1]. the
"undefined" one succeeds, but note that "t4058-diff-duplicates.sh"
triggers its assertions, they're just hidden by
"test_expect_failure" (see [3] for why).

1. https://lore.kernel.org/git/220725.861qu9oxl4.gmgdl@evledraar.gmail.com/
2. https://lore.kernel.org/git/YPClS0fj2HOJE5nH@coredump.intra.peff.net/
3. https://lore.kernel.org/git/cover-0.7-00000000000-20220318T002951Z-avarab@gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

This CI wil catch the memory corruption that made it to "master", see:
https://lore.kernel.org/git/220725.861qu9oxl4.gmgdl@evledraar.gmail.com/

 .github/workflows/main.yml | 6 ++++++
 ci/lib.sh                  | 6 ++++++
 2 files changed, 12 insertions(+)

diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index cd1f52692a5..4f59a7aa44c 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -251,6 +251,12 @@ jobs:
           - jobname: linux-leaks
             cc: gcc
             pool: ubuntu-latest
+          - jobname: SANITIZE=address
+            cc: gcc
+            pool: ubuntu-latest
+          - jobname: SANITIZE=undefined
+            cc: gcc
+            pool: ubuntu-latest
     env:
       CC: ${{matrix.vector.cc}}
       CC_PACKAGE: ${{matrix.vector.cc_package}}
diff --git a/ci/lib.sh b/ci/lib.sh
index f095519f8db..5ad60af2e7c 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -277,6 +277,12 @@ linux-leaks)
 	export SANITIZE=leak
 	export GIT_TEST_PASSING_SANITIZE_LEAK=true
 	;;
+SANITIZE=address)
+	export SANITIZE=address
+	;;
+SANITIZE=undefined)
+	export SANITIZE=undefined
+	;;
 esac
 
 MAKEFLAGS="$MAKEFLAGS CC=${CC:-cc}"
-- 
2.37.1.1107.g76fe5d1ed7c

Re: [PATCH] CI: add SANITIZE=[address|undefined] jobs

[Derrick Stolee]

On 7/26/2022 7:09 AM, Ævar Arnfjörð Bjarmason wrote:
> Add CI targets for SANITIZE=address and SANITIZE=undefined. The former
> would have caught a regression in 18bbc795fc5 (Merge branch
> 'gc/bare-repo-discovery', 2022-07-22) which made its way to
> "master"[1].
> 
> Per [2] the GitHub fork of git.git runs with these in CI, so it's
> already useful to some forks of this repository.

I'm a fan of adding additional sanitizer checks in our CI. Let's let
computers do the work for us here instead of relying on humans.

> Also per [2] we could use SANITIZE=address with some ASAN_OPTIONS
> instead of our SANITIZE=leak job added in 956d2e4639b (tests: add a
> test mode for SANITIZE=leak, run it in CI, 2021-09-23), but unifying
> those two with these new jobs would be a lot harder, so let's leave
> that for now.
>            - jobname: linux-leaks
>              cc: gcc
>              pool: ubuntu-latest
> +          - jobname: SANITIZE=address
> +            cc: gcc
> +            pool: ubuntu-latest
> +          - jobname: SANITIZE=undefined
> +            cc: gcc
> +            pool: ubuntu-latest

> @@ -277,6 +277,12 @@ linux-leaks)
>  	export SANITIZE=leak
>  	export GIT_TEST_PASSING_SANITIZE_LEAK=true
>  	;;
> +SANITIZE=address)
> +	export SANITIZE=address
> +	;;
> +SANITIZE=undefined)
> +	export SANITIZE=undefined
> +	;;

In both of these cases, we are breaking from the nearby pattern. These
jobs could be renamed to linux-address and linux-undefined to match the
linux-leaks job.

Alternatively, we could rename linux-leaks to SANITIZE=leak, since the
point is not to test the Linux platform but to use the additional runtime
checks (and Linux is the fasted CI platform).

Thanks,
-Stolee

Re: [PATCH] CI: add SANITIZE=[address|undefined] jobs

[Ævar Arnfjörð Bjarmason]

On Tue, Jul 26 2022, Derrick Stolee wrote:

> On 7/26/2022 7:09 AM, Ævar Arnfjörð Bjarmason wrote:
>> Add CI targets for SANITIZE=address and SANITIZE=undefined. The former
>> would have caught a regression in 18bbc795fc5 (Merge branch
>> 'gc/bare-repo-discovery', 2022-07-22) which made its way to
>> "master"[1].
>> 
>> Per [2] the GitHub fork of git.git runs with these in CI, so it's
>> already useful to some forks of this repository.
>
> I'm a fan of adding additional sanitizer checks in our CI. Let's let
> computers do the work for us here instead of relying on humans.

Thanks, good to have agreement on adding these CI runs.

>> Also per [2] we could use SANITIZE=address with some ASAN_OPTIONS
>> instead of our SANITIZE=leak job added in 956d2e4639b (tests: add a
>> test mode for SANITIZE=leak, run it in CI, 2021-09-23), but unifying
>> those two with these new jobs would be a lot harder, so let's leave
>> that for now.
>>            - jobname: linux-leaks
>>              cc: gcc
>>              pool: ubuntu-latest
>> +          - jobname: SANITIZE=address
>> +            cc: gcc
>> +            pool: ubuntu-latest
>> +          - jobname: SANITIZE=undefined
>> +            cc: gcc
>> +            pool: ubuntu-latest
>
>> @@ -277,6 +277,12 @@ linux-leaks)
>>  	export SANITIZE=leak
>>  	export GIT_TEST_PASSING_SANITIZE_LEAK=true
>>  	;;
>> +SANITIZE=address)
>> +	export SANITIZE=address
>> +	;;
>> +SANITIZE=undefined)
>> +	export SANITIZE=undefined
>> +	;;
>
> In both of these cases, we are breaking from the nearby pattern. These
> jobs could be renamed to linux-address and linux-undefined to match the
> linux-leaks job.
>
> Alternatively, we could rename linux-leaks to SANITIZE=leak[...]

I deliberately deviated from the "linux-leaks" pattern since it's a lot
more than just:

	make SANITIZE=leak test

I.e. we instrument what tests we run, skip some individual ones
etc. These are different in that we can run the entire set. I'd think
the reverse would make sense, i.e. one day if we run fully with
SANITIZE=leak enabled to rename that job to "SANITIZE=leak".

> [...], since the
> point is not to test the Linux platform but to use the additional runtime
> checks (and Linux is the fasted CI platform).

Strictly speaking these tests are platform-specific in that they require
us to take certain codepaths at runtime, so if we have any
platform-specific code, or code that's affected by compilation options
(say NO_REGEX=Y v.s. using the libc's) we might see failures on one
platform, but not another. Compilation flags also matter (e.g. -O0
v.s. -O3).

But I think for all of [leak,address,undefined] it's a sensible
trade-off for now to just pick one specific platform.

Since it's very unlikely that the resulting issues are OS-specific I
thought it made sense to leave "linux" out of it, just like we have
"pedantic", not "linux-pedantic", ditto "sparse" which is also
platform-specific around the edges.

Having said all that I really don't care much what we call these as long
as we get the test coverage, but I'll hold off on any possible re-roll
to see if others chime in about the bikeshed :)

Re: [PATCH] CI: add SANITIZE=[address|undefined] jobs

[Derrick Stolee]

On 7/26/22 9:33 AM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Tue, Jul 26 2022, Derrick Stolee wrote:
> 
>> On 7/26/2022 7:09 AM, Ævar Arnfjörð Bjarmason wrote:

>>>            - jobname: linux-leaks
>>>              cc: gcc
>>>              pool: ubuntu-latest
>>> +          - jobname: SANITIZE=address
>>> +            cc: gcc
>>> +            pool: ubuntu-latest
>>> +          - jobname: SANITIZE=undefined
>>> +            cc: gcc
>>> +            pool: ubuntu-latest
>>
>>> @@ -277,6 +277,12 @@ linux-leaks)
>>>  	export SANITIZE=leak
>>>  	export GIT_TEST_PASSING_SANITIZE_LEAK=true
>>>  	;;
>>> +SANITIZE=address)
>>> +	export SANITIZE=address
>>> +	;;
>>> +SANITIZE=undefined)
>>> +	export SANITIZE=undefined
>>> +	;;
>>
>> In both of these cases, we are breaking from the nearby pattern. These
>> jobs could be renamed to linux-address and linux-undefined to match the
>> linux-leaks job.
>>
>> Alternatively, we could rename linux-leaks to SANITIZE=leak[...]
> 
> I deliberately deviated from the "linux-leaks" pattern since it's a lot
> more than just:
> 
> 	make SANITIZE=leak test
> 
> I.e. we instrument what tests we run, skip some individual ones
> etc. These are different in that we can run the entire set. I'd think
> the reverse would make sense, i.e. one day if we run fully with
> SANITIZE=leak enabled to rename that job to "SANITIZE=leak".

Yes, SANITIZE=leak requires extra steps to make tests pass, but
as far as setting up the CI job it is extremely similar to the
two you are adding here. Their purpose is also very similar:
add compiler flags that would cause our test suite to fail when
certain memory issues occur. We just expect the SANITIZE=address
and SANITIZE=undefined to pass for every test in the test suite.

That's why I think renaming linux-leaks to SANITIZE=leak is the
best option.

Thanks,
-Stolee

Re: [PATCH] CI: add SANITIZE=[address|undefined] jobs

[Junio C Hamano]

Derrick Stolee <derrickstolee@github.com> writes:

>>            - jobname: linux-leaks
>>              cc: gcc
>>              pool: ubuntu-latest
>> +          - jobname: SANITIZE=address
>> +            cc: gcc
>> +            pool: ubuntu-latest
>> +          - jobname: SANITIZE=undefined
>> +            cc: gcc
>> +            pool: ubuntu-latest
>
>> @@ -277,6 +277,12 @@ linux-leaks)
>>  	export SANITIZE=leak
>>  	export GIT_TEST_PASSING_SANITIZE_LEAK=true
>>  	;;
>> +SANITIZE=address)
>> +	export SANITIZE=address
>> +	;;
>> +SANITIZE=undefined)
>> +	export SANITIZE=undefined
>> +	;;
>
> In both of these cases, we are breaking from the nearby pattern. These
> jobs could be renamed to linux-address and linux-undefined to match the
> linux-leaks job.
>
> Alternatively, we could rename linux-leaks to SANITIZE=leak, since the
> point is not to test the Linux platform but to use the additional runtime
> checks (and Linux is the fasted CI platform).

I tend to agree that in the existing linux-leaks job, the
"linux"-ness is much less important than the "leaks"-ness, so the
"alternative" might be slightly more preferable, but I do not mind
the renaming goes the other way, either.

Thanks for your good eyes and good taste.

Re: [PATCH] CI: add SANITIZE=[address|undefined] jobs

[Jeff King]

On Tue, Jul 26, 2022 at 01:09:13PM +0200, Ævar Arnfjörð Bjarmason wrote:

> Per [2] the GitHub fork of git.git runs with these in CI, so it's
> already useful to some forks of this repository.

Yeah, it has been helpful there and I think this is worth doing as part
of our CI. It's a lot of CPU versus running the test suite once on
Linux, but probably not compared to the overall cost of our current CI.

For the GitHub fork, the code-coverage issues you noticed were easy: we
only built one variant, so we could just test with those knobs. ;) But I
tend to agree with your approach here to just test on one platform,
which covers _most_ of the code. It's certainly better than the status
quo, and it strikes a nice balance of CPU versus coverage.

One thing I'd say...

> diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
> index cd1f52692a5..4f59a7aa44c 100644
> --- a/.github/workflows/main.yml
> +++ b/.github/workflows/main.yml
> @@ -251,6 +251,12 @@ jobs:
>            - jobname: linux-leaks
>              cc: gcc
>              pool: ubuntu-latest
> +          - jobname: SANITIZE=address
> +            cc: gcc
> +            pool: ubuntu-latest
> +          - jobname: SANITIZE=undefined
> +            cc: gcc
> +            pool: ubuntu-latest

There's really no reason to split the "address" and "undefined" builds
into two jobs. We expect them to pass, and if one fails, having the
results split is not likely to give any extra information. So I think
one job with SANITIZE=address,undefined is fine, and reclaims some of
the extra CPU we're spending.

-Peff

Re: [PATCH] CI: add SANITIZE=[address|undefined] jobs

[Ævar Arnfjörð Bjarmason]

On Wed, Jul 27 2022, Junio C Hamano wrote:

> Derrick Stolee <derrickstolee@github.com> writes:
>
>>>            - jobname: linux-leaks
>>>              cc: gcc
>>>              pool: ubuntu-latest
>>> +          - jobname: SANITIZE=address
>>> +            cc: gcc
>>> +            pool: ubuntu-latest
>>> +          - jobname: SANITIZE=undefined
>>> +            cc: gcc
>>> +            pool: ubuntu-latest
>>
>>> @@ -277,6 +277,12 @@ linux-leaks)
>>>  	export SANITIZE=leak
>>>  	export GIT_TEST_PASSING_SANITIZE_LEAK=true
>>>  	;;
>>> +SANITIZE=address)
>>> +	export SANITIZE=address
>>> +	;;
>>> +SANITIZE=undefined)
>>> +	export SANITIZE=undefined
>>> +	;;
>>
>> In both of these cases, we are breaking from the nearby pattern. These
>> jobs could be renamed to linux-address and linux-undefined to match the
>> linux-leaks job.
>>
>> Alternatively, we could rename linux-leaks to SANITIZE=leak, since the
>> point is not to test the Linux platform but to use the additional runtime
>> checks (and Linux is the fasted CI platform).
>
> I tend to agree that in the existing linux-leaks job, the
> "linux"-ness is much less important than the "leaks"-ness, so the
> "alternative" might be slightly more preferable, but I do not mind
> the renaming goes the other way, either.

I'm fine with it either way, but to add a prep patch to this series to
do s/linux-leaks/SANITIZE=leak/g would cause a semantic conflict with my
in-flight
https://lore.kernel.org/git/cover-v3-00.15-00000000000-20220727T230800Z-avarab@gmail.com/;
It refers to "linux-leaks" in newly added documentation.

So what would you prefer here, to take a lightly altered re-roll of this
(I think Jeff's suggestion of bundling "undefined" into one job makes
sense), or have it build on top of that otherwise unrelated series, or
just do the s/linux-leaks/.../g as an eventual follow-up?

Re: [PATCH] CI: add SANITIZE=[address|undefined] jobs

[Ævar Arnfjörð Bjarmason]

On Wed, Jul 27 2022, Jeff King wrote:

> On Tue, Jul 26, 2022 at 01:09:13PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> Per [2] the GitHub fork of git.git runs with these in CI, so it's
>> already useful to some forks of this repository.
>
> Yeah, it has been helpful there and I think this is worth doing as part
> of our CI. It's a lot of CPU versus running the test suite once on
> Linux, but probably not compared to the overall cost of our current CI.
>
> For the GitHub fork, the code-coverage issues you noticed were easy: we
> only built one variant, so we could just test with those knobs. ;) But I
> tend to agree with your approach here to just test on one platform,
> which covers _most_ of the code. It's certainly better than the status
> quo, and it strikes a nice balance of CPU versus coverage.

*nod*

> One thing I'd say...
>
>> diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
>> index cd1f52692a5..4f59a7aa44c 100644
>> --- a/.github/workflows/main.yml
>> +++ b/.github/workflows/main.yml
>> @@ -251,6 +251,12 @@ jobs:
>>            - jobname: linux-leaks
>>              cc: gcc
>>              pool: ubuntu-latest
>> +          - jobname: SANITIZE=address
>> +            cc: gcc
>> +            pool: ubuntu-latest
>> +          - jobname: SANITIZE=undefined
>> +            cc: gcc
>> +            pool: ubuntu-latest
>
> There's really no reason to split the "address" and "undefined" builds
> into two jobs. We expect them to pass, and if one fails, having the
> results split is not likely to give any extra information. So I think
> one job with SANITIZE=address,undefined is fine, and reclaims some of
> the extra CPU we're spending.

I'll do that in a re-roll, pending a resolution of the naming discussion
at:
https://lore.kernel.org/git/220728.86sfmljhyx.gmgdl@evledraar.gmail.com/

But note that it *does* give you extra information to split them up
currently, i.e. the "test_expect_failure" that you get with "undefined"
isn't conflated with the non-changes that SANITIZE=address flags (sans
outstanding recent breakage) in the test report.

But just having that "TODO" test sitting there will suck less than
potentially having CI run much longer, or taking up resources from
concurrent CI runs, so I'll do this.

We also leave a lot of CI performance on the table by e.g. doing "chain
lint" in every single test run (except Windows), there *are* platform
edge-cases there like with SANITIZE=address, but I wonder if we should
just declare it good enough to do it in 1-2 jobs.

Ditto TEST_NO_MALLOC_CHECK=1 & --no-bin-wrappers, but we can think about
all of those some other time....

Thanks for reviewing this.

Re: [PATCH] CI: add SANITIZE=[address|undefined] jobs

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> I'm fine with it either way, but to add a prep patch to this series to
> do s/linux-leaks/SANITIZE=leak/g would cause a semantic conflict with my
> in-flight
> https://lore.kernel.org/git/cover-v3-00.15-00000000000-20220727T230800Z-avarab@gmail.com/;
> It refers to "linux-leaks" in newly added documentation.

If you mean "in-flight" something that is not in 'next', it may have
to be rerolled on top of this new topic.

If something is already in 'next' and we are committed to see it
through down to 'master', then this new topic may need to depend on
it.

Re: [PATCH] CI: add SANITIZE=[address|undefined] jobs

[Jeff King]

On Thu, Jul 28, 2022 at 06:54:37PM +0200, Ævar Arnfjörð Bjarmason wrote:

> > There's really no reason to split the "address" and "undefined" builds
> > into two jobs. We expect them to pass, and if one fails, having the
> > results split is not likely to give any extra information. So I think
> > one job with SANITIZE=address,undefined is fine, and reclaims some of
> > the extra CPU we're spending.
> 
> I'll do that in a re-roll, pending a resolution of the naming discussion
> at:
> https://lore.kernel.org/git/220728.86sfmljhyx.gmgdl@evledraar.gmail.com/

Thanks. FWIW, I have no opinion on the job naming. ;)

> But note that it *does* give you extra information to split them up
> currently, i.e. the "test_expect_failure" that you get with "undefined"
> isn't conflated with the non-changes that SANITIZE=address flags (sans
> outstanding recent breakage) in the test report.
> 
> But just having that "TODO" test sitting there will suck less than
> potentially having CI run much longer, or taking up resources from
> concurrent CI runs, so I'll do this.

My hope would be that we'd identify and fix cases where this flagged,
even in expect_failure, and so any state like you describe would be
temporary. In the long term, we should be able to assume that these
don't trigger warnings in the existing code base, and optimize in that
direction.

> We also leave a lot of CI performance on the table by e.g. doing "chain
> lint" in every single test run (except Windows), there *are* platform
> edge-cases there like with SANITIZE=address, but I wonder if we should
> just declare it good enough to do it in 1-2 jobs.

I'd be fine with that, but I think chain lint isn't actually that
expensive. The original in-shell bits are super cheap. The extra
sed process is measurable, but I think I blunted the worst of it in the
2d86a96220 (t: avoid sed-based chain-linting in some expensive cases,
2021-05-13).

Still, that patch should make it easy to time things just by setting
GIT_TEST_CHAIN_LINT_HARDER=0 in various jobs. It does seem to buy ~100s
of CPU time per test run on my Linux box. That's not a lot in the grand
scheme, but perhaps adds up. And I could believe it's much worse on
Windows. Maybe worth seeing how it performs in the actual CI
environments.

> Ditto TEST_NO_MALLOC_CHECK=1 & --no-bin-wrappers, but we can think about
> all of those some other time....

I'd be surprised if the malloc checking itself is all that expensive,
though it does look like we call getconf and expr once per test there
for setup. We could almost certainly hoist that out and call it once per
script.

-Peff

Re: [PATCH] CI: add SANITIZE=[address|undefined] jobs

[Jeff King]

On Thu, Jul 28, 2022 at 05:09:38PM -0400, Jeff King wrote:

> > Ditto TEST_NO_MALLOC_CHECK=1 & --no-bin-wrappers, but we can think about
> > all of those some other time....
> 
> I'd be surprised if the malloc checking itself is all that expensive,
> though it does look like we call getconf and expr once per test there
> for setup. We could almost certainly hoist that out and call it once per
> script.

Nope, I was totally wrong here. Running with TEST_NO_MALLOC_CHECK=1 does
indeed make a big difference:

  Benchmark 1: make test
    Time (mean ± σ):     67.486 s ±  3.339 s    [User: 622.643 s, System: 409.951 s]
    Range (min … max):   63.634 s … 69.550 s    3 runs
   
  Benchmark 2: TEST_NO_MALLOC_CHECK=1 make test
    Time (mean ± σ):     57.596 s ±  0.899 s    [User: 577.273 s, System: 291.627 s]
    Range (min … max):   57.072 s … 58.634 s    3 runs

Eliminating the extra per-test processes with the patch below helps a
little, but most of the effort really is (presumably) going to the
actual malloc checking code:

  Benchmark 1: make test [nb: after applying patch...]
  Time (mean ± σ):     67.091 s ±  0.385 s    [User: 599.382 s, System: 410.781 s]
  Range (min … max):   66.648 s … 67.343 s    3 runs

Curious that most of the CPU time seems to go into system time. I'd have
thought it was extra internal malloc debugging bits, but maybe it is
allocating mmap/brk calls in a less efficient way. I dunno how any of it
actually works.

-Peff

---
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 55857af601..303fbe8ecc 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -557,14 +557,20 @@ then
 		: nothing
 	}
 else
+	if _GLIBC_VERSION=$(getconf GNU_LIBC_VERSION 2>/dev/null) &&
+	   _GLIBC_VERSION=${_GLIBC_VERSION#"glibc "} &&
+	    expr 2.34 \<= "$_GLIBC_VERSION" >/dev/null
+	then
+		USE_LIBC_MALLOC_DEBUG=t
+	else
+		USE_LIBC_MALLOC_DEBUG=
+	fi
 	setup_malloc_check () {
 		local g
 		local t
 		MALLOC_CHECK_=3	MALLOC_PERTURB_=165
 		export MALLOC_CHECK_ MALLOC_PERTURB_
-		if _GLIBC_VERSION=$(getconf GNU_LIBC_VERSION 2>/dev/null) &&
-		   _GLIBC_VERSION=${_GLIBC_VERSION#"glibc "} &&
-		   expr 2.34 \<= "$_GLIBC_VERSION" >/dev/null
+		if test -n "$USE_LIBC_MALLOC_DEBUG"
 		then
 			g=
 			LD_PRELOAD="libc_malloc_debug.so.0"

Re: [PATCH] CI: add SANITIZE=[address|undefined] jobs

[Ævar Arnfjörð Bjarmason]

On Thu, Jul 28 2022, Jeff King wrote:

> On Thu, Jul 28, 2022 at 06:54:37PM +0200, Ævar Arnfjörð Bjarmason wrote:
> [...]
>> We also leave a lot of CI performance on the table by e.g. doing "chain
>> lint" in every single test run (except Windows), there *are* platform
>> edge-cases there like with SANITIZE=address, but I wonder if we should
>> just declare it good enough to do it in 1-2 jobs.
>
> I'd be fine with that, but I think chain lint isn't actually that
> expensive. The original in-shell bits are super cheap. The extra
> sed process is measurable, but I think I blunted the worst of it in the
> 2d86a96220 (t: avoid sed-based chain-linting in some expensive cases,
> 2021-05-13).
>
> Still, that patch should make it easy to time things just by setting
> GIT_TEST_CHAIN_LINT_HARDER=0 in various jobs. It does seem to buy ~100s
> of CPU time per test run on my Linux box. That's not a lot in the grand
> scheme, but perhaps adds up. And I could believe it's much worse on
> Windows. Maybe worth seeing how it performs in the actual CI
> environments.
>
>> Ditto TEST_NO_MALLOC_CHECK=1 & --no-bin-wrappers, but we can think about
>> all of those some other time....
>
> I'd be surprised if the malloc checking itself is all that expensive,
> though it does look like we call getconf and expr once per test there
> for setup. We could almost certainly hoist that out and call it once per
> script.

I posted some benchmarks for these a while ago:
https://lore.kernel.org/git/220405.86k0c3lt2l.gmgdl@evledraar.gmail.com/

YMMV, but these make a big difference for some tests, although less for
a whole run, as you point out disabling chain lint least impact.

For me a full test suite run is:

* TEST_NO_MALLOC_CHECK=1 --no-bin-wrappers --no-chain-lint:
	real    1m55.600s
	user    8m48.039s
	sys     3m17.776s
* [no opts, TEST_NO_MALLOC_CHECK=0 & --bin-wrappers && --chain-lint are on]
	real    2m18.126s
	user    10m40.230s
	sys     4m7.152s

So far from the >2x speedup you can get with some tests, but a big
difference nonetheless.