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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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

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