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

[Jeff King <peff@peff.net>]

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