Re: [PATCH] Makefile: fix bugs in coccicheck and speed it up

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Fri, Mar 05 2021, Jeff King wrote:

I sent out a v2 that should address your feedback in :
https://lore.kernel.org/git/20210305170724.23859-1-avarab@gmail.com/

Just brief notes:

> On Tue, Mar 02, 2021 at 09:51:03PM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>> What we were doing before was processing all our *.c files, and for
>> each of those *.c files we'd recursively look around for includes and
>> see if we needed to search/replace in those too.
>> 
>> That we did that dates back to [1] when we were only processing *.c
>> files, and it was always very redundant. We'd e.g. visit the likes of
>> strbuf.h lots of times since it's widely used as an include.
>
> OK. I don't offhand know if processing the includes along with the C
> files buys us anything else. Coccinelle's behavior is often quite a
> mystery, but if we think this produces the same results with less time,
> I'm for it.
>
> BTW, this "dates back to when we were only processing *.c files"
> statement confused me a bit. Doesn't that include the current state
> before this patch?  I.e., this hunk:
>
>> -FOUND_C_SOURCES = $(filter %.c,$(shell $(FIND_SOURCE_FILES)))
>> +FOUND_C_SOURCES = $(filter %.c %.h,$(shell $(FIND_SOURCE_FILES)))
>
> seems to be an important part of the change, without which moving away
> from --all-includes would be wrong.

Updated in the new 2/4 commit message.

>> Then in the most recent attempt to optimize coccicheck in [2] this
>> anti-pattern finally turned into a bug.
>>
>> Namely: before this change, if your coccicheck rule applied to
>> e.g. making a change in strbuf.h itself we'd get *lots* of duplicate
>> hunks applying the exact same change, as concurrent spatch processes
>> invoked by xargs raced one another. In one instance I ended up with 27
>> copies of the same hunk in a strbuf.patch.
>> 
>> Setting SPATCH_BATCH_SIZE=0 and processing all the files in one giant
>> batch mitigated this. I suspect the author of [2] either mostly ran in
>> that mode, or didn't test on changes that impacted widely used header
>> files.
>
> This "turned into a bug" I didn't understand, though. That commit [2]
> just switched from always feeding files one at a time to letting you
> feed more than one.  So yes, feeding all at once (with
> SPATCH_BATCH_SIZE=0) mitigated the bug. But wouldn't any bug that shows
> up with SPATCH_BATCH_SIZE=1 have already existed previously?
>
> IOW, I don't think the batch-size stuff made anything worse. It made one
> specific case better, but that was not even its purpose.
>
> That is maybe splitting hairs, but I want to make sure I understand all
> of what you're claiming here. But also...

Explained in the new 2/4. Your commit introduced a regression, because
spatch does racy magic background locking, running N of them at a time
applying the same rule tripped it up.

I suspect it was racy before, but we didn't have overlapping rules in
multiple *.cocci files.

>> I'm entirely removing SPATCH_BATCH_SIZE. If you want to tweak it you
>> can tweak SPATCH_XARGS_FLAGS to e.g. "-n 256", or "-P 4 -n 128". But
>> in my testing it isn't worth it to tweak SPATCH_XARGS_FLAGS for a full
>> "make coccicheck".
>
> You hard-code this to 32 now. But I'm not sure if that's going to be the
> universal best value.
>
> Applying just part of your patch, but leaving SPATCH_BATCH_SIZE in
> place, like so:
>
> diff --git a/Makefile b/Makefile
> index dfb0f1000f..88cb157547 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1201,7 +1201,7 @@ SP_EXTRA_FLAGS = -Wno-universal-initializer
>  # For the 'coccicheck' target; setting SPATCH_BATCH_SIZE higher will
>  # usually result in less CPU usage at the cost of higher peak memory.
>  # Setting it to 0 will feed all files in a single spatch invocation.
> -SPATCH_FLAGS = --all-includes --patch .
> +SPATCH_FLAGS = --no-includes --patch .
>  SPATCH_BATCH_SIZE = 1
>  
>  include config.mak.uname
> @@ -2859,7 +2859,7 @@ check: config-list.h command-list.h
>  		exit 1; \
>  	fi
>  
> -FOUND_C_SOURCES = $(filter %.c,$(shell $(FIND_SOURCE_FILES)))
> +FOUND_C_SOURCES = $(filter %.c %.h,$(shell $(FIND_SOURCE_FILES)))
>  COCCI_SOURCES = $(filter-out $(THIRD_PARTY_SOURCES),$(FOUND_C_SOURCES))
>  
>  %.cocci.patch: %.cocci $(COCCI_SOURCES)
>
> Then I get this with various batch sizes (using "make -j16" on an 8-core
> machine, so all of the patches are being run at the same time):
>
>   size |  real  |  user  | sys
>   -------------------------------
>     1  |  1m28s | 10m00s | 0m56s
>     2  |  1m03s |  7m49s | 0m33s
>     4  |  0m51s |  6m28s | 0m20s
>     8  |  0m45s |  6m02s | 0m14s
>    16  |  0m45s |  6m17s | 0m10s
>    32  |  0m44s |  6m33s | 0m07s
>    64  |  0m45s |  6m48s | 0m06s
>     0  |  1m08s | 10m08s | 0m03s
>
> So there's a sweet spot at 8. Doing "32" isn't that much worse (runtime
> is about the same, but we use more CPU), but it gets progressively worse
> as the batch size increases.
>
> That's the same sweet spot before your patch, too, btw. I know because I
> happened to be timing it the other day, as coccinelle 1.1.0 hit debian
> unstable. When I originally wrote the SPATCH_BATCH_SIZE feature, I think
> I was using 1.0.4 or 1.0.7. Back then SPATCH_BATCH_SIZE=0 was a clear
> win, assuming you had the memory. But in 1.0.8 it ran for many minutes
> without finishing. I found back then that "2" was the sweet spot. But
> now it's "8".
>
> All of which is to say: the timing difference between my "8" and "32"
> cases isn't that exciting. But the performance of spatch has been
> sufficiently baffling to me that I think it's worth keeping the knob.
>
> Your XARGS_FLAGS does accomplish something similar (and is in fact more
> flexible, though at the cost of being less abstract).  I'm OK to replace
> one with the other, but shouldn't it happen in a separate patch? It's
> completely orthogonal to the --no-includes behavior.

*nod*, set to a default of 8 in the new 4/4.

>> I'm also the whole "cat $@.log" introduced in [3]. Since we don't call
>> this in a loop anymore (and xargs will early-exit) we can just rely on
>> standard V=1 for debugging issues.
>
> I think this is missing the point of the cat. We've redirected spatch's
> stderr to the logfile. So if there's an error, you have no idea what it
> was without manually figuring out which file to cat. And V=1 doesn't
> help that.
>
> We could stop doing that redirection, but the problem is that spatch is
> very verbose. So the point was to store the output but only dump it
> when we see an error.
>
> So this part of the patch seems like a pure regression to me. E.g.,
> introduce a syntax error like:
>
> diff --git a/contrib/coccinelle/free.cocci b/contrib/coccinelle/free.cocci
> index 4490069df9..c6c6562a0a 100644
> --- a/contrib/coccinelle/free.cocci
> +++ b/contrib/coccinelle/free.cocci
> @@ -11,7 +11,7 @@ expression E;
>    free(E);
>  
>  @@
> -expression E;
> +expression E
>  @@
>  - free(E);
>  + FREE_AND_NULL(E);
>
> and run "make coccicheck" before and after your patch:
>
>   [before]
>   $ make coccicheck
>   GIT_VERSION = 2.31.0.rc1.8.gbe7935ed8b.dirty
>       SPATCH contrib/coccinelle/free.cocci
>   init_defs_builtins: /usr/bin/../lib/coccinelle/standard.h
>   meta: parse error: 
>     File "contrib/coccinelle/free.cocci", line 15, column 0, charpos = 99
>     around = '@@',
>     whole content = @@
>
>   xargs: spatch: exited with status 255; aborting
>   make: *** [Makefile:2866: contrib/coccinelle/free.cocci.patch] Error 1
>
>   [after]
>   $ make coccicheck
>     SPATCH contrib/coccinelle/free.cocci
>   make: *** [Makefile:2875: contrib/coccinelle/free.cocci.patch] Error 124

The "cat $@.log" is back, but there's still some issues with it on
master now (I didn't change this) it'll spew a lot at you with xargs
since we emit the whole error output for every file, seemingly, before
cat-ing the file:

    make -j1 coccicheck SPATCH_FLAGS=--blahblah V=1 2>&1|grep -i example.*use|wc -l
    464

Well, it's a bit better now on my series with a default batch size of 8:

    $ make -j1 coccicheck SPATCH_FLAGS=--blahblah V=1 2>&1|grep -i example.*use|wc -l
    88

I got tired of dealing with the combination of shellscritp and make for
the day. But maybe something to do as a follow-up if anyone's
interested.