Re: [PATCH] cocci: remove 'unused.cocci'

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: May 1, 2023, 13:27 UTC
Message-ID: <230501.864jowjh15.gmgdl@evledraar.gmail.com>
In-Reply-To: <20230420205350.600760-1-szeder.dev@gmail.com>

On Thu, Apr 20 2023, SZEDER Gábor wrote:

Show 32 quoted lines

> When 'unused.cocci' was added in 4f40f6cb73 (cocci: add and apply a
> rule to find "unused" strbufs, 2022-07-05) it found three unused
> strbufs, and when it was generalized in the next commit it managed to
> find an unused string_list as well.  That's four unused variables in
> over 17 years, so apparently we rarely make this mistake.
>
> Unfortunately, applying 'unused.cocci' is quite expensive, e.g. it
> increases the from-scratch runtime of 'make coccicheck' by over 5:30
> minutes or over 160%:
>
>   $ make -s cocciclean
>   $ time make -s coccicheck
>       * new spatch flags
>
>   real    8m56.201s
>   user    0m0.420s
>   sys     0m0.406s
>   $ rm contrib/coccinelle/unused.cocci contrib/coccinelle/tests/unused.*
>   $ make -s cocciclean
>   $ time make -s coccicheck
>       * new spatch flags
>
>   real    3m23.893s
>   user    0m0.228s
>   sys     0m0.247s
>
> That's a lot of runtime spent for not much in return, and arguably an
> unused struct instance sneaking in is not that big of a deal to
> justify the significantly increased runtime.
>
> Remove 'unused.cocci', because we are not getting our CPU cycles'
> worth.

It wasn't something I intended at the time, but arguably the main use of this rule since it was added was that it served as a canary for the tree becoming completely broken with coccinelle, due to adding C syntax it didn't understand: https://lore.kernel.org/git/220825.86ilmg4mil.gmgdl@evledraar.gmail.com/

So, whatever you think of of how worthwhile it is to spot unused variables, I think that weighs heavily in its favor. There *are* other ways to detect those sorts of issues, but as it's currently our only canary for that issue I don't thin we should remove it.

If we hadn't had unused.cocci we wouldn't be able to apply rules the functions that use "UNUSED", which have increased a lot in number since then, and we wouldn't have any way of spotting similar parsing issues.

But it's unfortunate that it's this slow in the from-scratch case.

When we last discussed this I pointed out to you that the main contribution to the runtime of unused.cocci is parsing builtin/{log,rebase}.c is pathalogical, but in your reply to that you seem to not have spotted that (or glossed over it): https://lore.kernel.org/git/20220831180526.GA1802@szeder.dev/

When I test this locally, doing:

	time make contrib/coccinelle/unused.cocci.patch SPATCH=spatch SPATCH_USE_O_DEPENDENCIES=

Takes ~2m, but if I first do:

	>builtin/log.c; >builtin/rebase.c

It takes ~1m.

So, even without digging into those issues, if we just skipped those two files we'd speed this part up by 100%.

I think such an approach would be much better than just removing this outright, which feels rather heavy-handed.

We could formalize that by creating a "coccicheck-full" category or whatever, just as we now have "coccicheck-pending".

Then I and the CI could run "full", and you could run "coccicheck" (or maybe we'd call that "coccicheck-cheap" or something).

I also submitted patches to both make "coccicheck" incremental, and added an "spatchcache", both of which have since been merged (that tool is in contrib/).

I understand from previous discussion that you wanted to use "make -s" all the time, but does your use-case also preclude using spatchcache?

I run "coccicheck" a lot, and haven't personally be bothered by this particular slowdown since that got merged, since it'll only affect me in the cases where builtin/{log,rebase}.c and a small list of other files are changed, and it's otherwise unnoticeable.

It would also be rather trivial to just add some way to specify patterns on the "make" command-line that we'd "$(filter-out)", would that also address your particular use-case? I.e.:

	make coccicheck COCCI_RULES_EXCLUDE=*unused*

Or whatever.