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

From: Jeff King <peff@peff.net>
Date: Jul 28, 2022, 21:09 UTC
Message-ID: <YuL7EotrIpnOn5BT@coredump.intra.peff.net>
In-Reply-To: <220728.86o7x9jhrp.gmgdl@evledraar.gmail.com>

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

Show 9 quoted lines

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

Show 8 quoted lines

> 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