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 [off-list ref]
 

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 <redacted>