Re: [PATCH 3/5] cocci: make "coccicheck" rule incremental

From: "SZEDER Gábor" <szeder.dev@gmail.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
	Jeff King <peff@peff.net>
Subject: Re: [PATCH 3/5] cocci: make "coccicheck" rule incremental
Date: Thu, 25 Aug 2022 21:44:18 +0200	[thread overview]
Message-ID: <20220825194418.GI1735@szeder.dev> (raw)
In-Reply-To: <patch-3.5-6fa83695f1f-20220825T141212Z-avarab@gmail.com>

On Thu, Aug 25, 2022 at 04:36:15PM +0200, Ævar Arnfjörð Bjarmason wrote:
> * Since we create a single "*.cocci.patch+" we don't know where to
>   pick up where we left off. Note that (per [1]) we've had a race
>   condition since 960154b9c17 (coccicheck: optionally batch spatch
>   invocations, 2019-05-06) which might result in us producing corrupt
>   patches to to the concurrent appending to "$@+" in the pre-image.
> 
>   That bug is now fixed.

There is no bug, because there is no concurrent appending to "$@+".
The message you mention seems to be irrelevant, as it talks about
'xargs -P', but the invocation in '%.cocci.patch' targets never used
'-P'.

> Which is why we'll not depend on $(FOUND_H_SOURCES) but the *.o file
> corresponding to the *.c file, if it exists already. This means that
> we can do:
> 
>     make all
>     make coccicheck
>     make -W column.h coccicheck
> 
> By depending on the *.o we piggy-back on
> COMPUTE_HEADER_DEPENDENCIES. See c234e8a0ecf (Makefile: make the
> "sparse" target non-.PHONY, 2021-09-23) for prior art of doing that
> for the *.sp files. E.g.:
> 
>     make contrib/coccinelle/free.cocci.patch
>     make -W column.h contrib/coccinelle/free.cocci.patch
> 
> Will take around 15 seconds for the second command on my 8 core box if
> I didn't run "make" beforehand to create the *.o files. But around 2
> seconds if I did and we have those "*.o" files.
> 
> Notes about the approach of piggy-backing on *.o for dependencies:
> 
> * It *is* a trade-off since we'll pay the extra cost of running the C
>   compiler, but we're probably doing that anyway.

This assumption doesn't hold, and I very much dislike the idea of
depending on *.o files:

  - Our static-analysis CI job doesn't build Git, now it will have to.

  - I don't have Coccinelle installed, because my distro doesn't ship
    it, and though the previous release did ship it, it was outdated.
    Instead I use Coccinelle's most recent version from a container
    which doesn't contain any build tools apart from 'make' for 'make
    coccicheck'.

    With this patch series I can't use this containerized Coccinelle
    at all, because even though I've already built git on the host,
    the dependency on *.o files triggers a BUILD-OPTIONS check during
    'make coccicheck', and due to the missing 'curl-config' the build
    options do differ, triggering a rebuild, which in the absence of a
    compiler fails.

    And then the next 'make' on the host will have to rebuild
    everything again...

>  * We can take better advantage of parallelism, while making sure that
>    we don't racily append to the contrib/coccinelle/swap.cocci.patch
>    file from multiple workers.
> 
>    Before this change running "make coccicheck" would by default end
>    up pegging just one CPU at the very end for a while, usually as
>    we'd finish whichever *.cocci rule was the most expensive.
> 
>    This could be mitigated by combining "make -jN" with
>    SPATCH_BATCH_SIZE, see 960154b9c17 (coccicheck: optionally batch
>    spatch invocations, 2019-05-06). But doing so required careful
>    juggling, as e.g. setting both to 4 would yield 16 workers.

No, setting both to 4 does yield 4 workers.

SPATCH_BATCH_SIZE has nothing to do with parallelism; it is merely the
number of C source files that we pass to a single 'spatch' invocation,
but for any given semantic patch it's still a sequential loop.