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@xxxxxxxxxxxxxxxxxxx/ 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@xxxxxxxxxx/ 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.
