Re: [PATCH v2 0/4] Makefile/coccicheck: fix bugs and speed it up
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Mar 6, 2021, 17:27 UTC
- Message-ID
- <87ft18tcog.fsf@evledraar.gmail.com>
- In-Reply-To
- <3aac381e-2ce9-e35e-498c-9c26df235aed@web.de>
On Fri, Mar 05 2021, René Scharfe. wrote:
Show 79 quoted lines
> Am 05.03.21 um 18:07 schrieb Ævar Arnfjörð Bjarmason: >> 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...