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

[Denton Liu <liu.denton@gmail.com>]

Hi Ævar,

On Tue, Mar 02, 2021 at 09:51:03PM +0100, Ævar Arnfjörð Bjarmason wrote:
> I've often wondered why "make coccicheck" takes so long. This change
> speeds it up by ~2x and makes it use much less memory. Or a reduction
> of a max of around ~2GB per-process (under the old
> SPATCH_BATCH_SIZE=0) to around ~200MB.
> 
> Running the full "make coccicheck" now takes ~50 seconds with -j8 on
> my machine, v.s. ~2x of that before. I've got 64GB of memory on that
> machine, or it would be much slower.
> 
> Why has it been so slow? Because I think we've always been running it
> in entirely the wrong mode for what we wanted, and much of the
> previous fixing of this target has involved re-arranging the deck
> chairs on that particular Titanic.
> 
> What we really want to do with coccicheck is to do search/replacements
> in all our *.c and *.h files. This is now what we do, and we'll
> process a default of 64 files at a time.
> 
> 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.
> 
> 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.
> 
> So since we're going to want to process all our *.c and *.h let's just
> do that, and drop --all-includes for --no-includes. It's not spatch's
> job to find our sources, we're doing that. If someone is manually
> tweaking COCCI_SOURCES they can just tweak SPATCH_FLAGS too.

This is a very clean and well-done analysis.

I've also timed my runs and the effects are really obvious:

Before:

	$ time make -j7 coccicheck
	    SPATCH contrib/coccinelle/array.cocci
	    SPATCH contrib/coccinelle/commit.cocci
	    SPATCH contrib/coccinelle/free.cocci
	    SPATCH contrib/coccinelle/hashmap.cocci
	    SPATCH contrib/coccinelle/object_id.cocci
	    SPATCH contrib/coccinelle/preincr.cocci
	    SPATCH contrib/coccinelle/strbuf.cocci
	    SPATCH contrib/coccinelle/swap.cocci
	    SPATCH contrib/coccinelle/xstrdup_or_null.cocci

	real    3m15.215s
	user    17m36.779s
	sys     1m4.262s

After:

	$ time make -j7 coccicheck
	GIT_VERSION = 2.31.0.rc1.1.gd8c3638591
	    SPATCH contrib/coccinelle/array.cocci
	    SPATCH contrib/coccinelle/commit.cocci
	    SPATCH contrib/coccinelle/flex_alloc.cocci
	    SPATCH contrib/coccinelle/free.cocci
	    SPATCH contrib/coccinelle/hashmap.cocci
	    SPATCH contrib/coccinelle/object_id.cocci
	    SPATCH contrib/coccinelle/preincr.cocci
	    SPATCH contrib/coccinelle/qsort.cocci
	    SPATCH contrib/coccinelle/strbuf.cocci
	    SPATCH contrib/coccinelle/swap.cocci
	    SPATCH contrib/coccinelle/xstrdup_or_null.cocci

	real    1m14.070s
	user    7m40.644s
	sys     0m13.689s

so this gets my

	Tested-by: Denton Liu <liu.denton@gmail.com>
 
> 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".
> 
> I'm also the whole "cat $@.log" introduced in [3]. Since we don't call

I'm also... what? :)

> this in a loop anymore (and xargs will early-exit) we can just rely on
> standard V=1 for debugging issues.
> 
> 1. a9a884aea5 (coccicheck: use --all-includes by default, 2016-09-30)
> 2. 960154b9c1 (coccicheck: optionally batch spatch invocations,
>    2019-05-06)
> 3. f5c2bc2b96 (Makefile: detect errors in running spatch, 2017-03-10)
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>