Re: [PATCH v2 0/4] Makefile/coccicheck: fix bugs and speed it up
From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2021-03-06 17:29:06
On Fri, Mar 05 2021, René Scharfe. wrote:
quoted hunk ↗ jump to hunk
Am 05.03.21 um 18:07 schrieb Ævar Arnfjörð Bjarmason:quoted
Addresses feedback on v1: - The removal of the "cat $@.log" is gone. - Split up into N changes. - Explained why 960154b9c17 (coccicheck: optionally batch spatch invocations, 2019-05-06) broke things and produced duplicate hunks in 2/4: tl;dr: spatch does its own locking etc., xargs -n trips it up. - Set number of batch processes to 8, as suggested by Jeff King. Ævar Arnfjörð Bjarmason (4): Makefile/coccicheck: add comment heading for all SPATCH flags Makefile/coccicheck: speed up and fix bug with duplicate hunks Makefile/coccicheck: allow for setting xargs concurrency Makefile/coccicheck: set SPATCH_BATCH_SIZE to 8 Makefile | 34 +++++++++++++++++++++++++--------- 1 file changed, 25 insertions(+), 9 deletions(-)This speeds up "make coccicheck" after "make clean" as advertized for me: before: 572.64s user 33.08s system 622% cpu 1:37.30 total after: 195.40s user 9.97s system 629% cpu 32.612 total However, it also misses several conversions that coccicheck generated without this series for the example added by the patch at the bottom. Here's the difference of diffstats (< before, > after): $ diff <(diffstat b | sort) <(diffstat a | sort) | grep '^[<>]' | sort -k2 -k1 > 27 files changed, 55 insertions(+), 57 deletions(-) < 36 files changed, 70 insertions(+), 71 deletions(-) < attr.c | 2 +- > blame.c | 15 +++++++-------- < blame.c | 17 ++++++++--------- < bloom.c | 2 +- < cache-tree.c | 2 +- > combine-diff.c | 18 +++++++++--------- < combine-diff.c | 20 ++++++++++---------- < decorate.c | 2 +- < diffcore-rename.c | 2 +- > diffcore-rename.c | 3 +-- < ewah/bitmap.c | 2 +- < hashmap.c | 2 +- > midx.c | 4 ++-- < midx.c | 8 ++++---- < pack-objects.c | 2 +- < pathspec.c | 2 +- > read-cache.c | 2 +- < read-cache.c | 4 ++-- > ref-filter.c | 2 +- < ref-filter.c | 4 ++-- < remote.c | 2 +- That's with the current spatch version 1.1.0-00072-g3dc5d027.diff --git a/contrib/coccinelle/array.cocci b/contrib/coccinelle/array.cocci index 46b8d2ee11..1f26da007a 100644 --- a/contrib/coccinelle/array.cocci +++ b/contrib/coccinelle/array.cocci@@ -88,3 +88,16 @@ expression n; @@ - ptr = xmalloc((n) * sizeof(T)); + ALLOC_ARRAY(ptr, n); + +@@ +type T; +T *ptr; +expression n != 1; +@@ +( +- ptr = xcalloc(n, \( sizeof(*ptr) \| sizeof(T) \)); ++ CALLOC_ARRAY(ptr, n); +| +- ptr = xcalloc(\( sizeof(*ptr) \| sizeof(T) \), n); ++ CALLOC_ARRAY(ptr, n); +)
Well spotted, the issue is that you're using a "type" in the rule, or well, rather that I don't know much about *.cocci and didn't think to test that. So because of s/--all-includes/--no-includes/ we don't include any includes, and thus don't know about the type, and thus can't match on this. So obviously a bad bug, and I'll need to re-roll this, but any fix I've been able to come up with (playing with other cocci options) takes away most of the speed gains. Well, there's the option of e.g. injecting options if "grep -q ^type <cocci-file>" is true, which would work for the current input. Do these sorts of rules really benefit that much from the type v.s. expression? If yes we'll obviously need to support it, but if (and I haven't looked closely) we can equally rewrite them with "expression" (or it would be good enough) we could be quite a bit faster by default...