On Wed, Aug 31 2022, SZEDER Gábor wrote: > On Fri, Aug 26, 2022 at 09:46:54AM -0700, Junio C Hamano wrote: >> Jeff King <peff@xxxxxxxx> writes: >> >> >> But the fix here isn't to delete unused.cocci, but to hold off on the >> >> UNUSEwork D() patches until we figure out how to make coccinelle jive with >> >> them. >> > >> > Yeah, my general skepticism and disappointment above notwithstanding, >> > this seems like the best path forward from here. I tried a few other >> > tricks (like --macro-file and --iso-file), but if its parser chokes, I >> > don't think there's much we can do about it. Even if we wrote a patch to >> > coccinelle itself (and I have no interest in doing that myself), it >> > would take a while to become available. >> >> If it is just a single unused.cocci, I would actually think removing >> it would be a much better path forward. UNUSED() that renames to >> help folks without checking compilers would help noticing bad code >> much earlier than unused.cocci many contributors are not running >> themselves anyway. > > Here is another reason for the removal of 'unused.cocci': it's very > costly to apply that semantic patch to the whole code base. > > make SPATCH_BATCH_SIZE=32 contrib/coccinelle/unused.cocci.patch > > takes 440s on my machine, whereas the second slowest 'object_id.cocci' > takes only 56s [1]. Applying 'unused.cocci' to some of our source files > individually takes well over a minute: > > $ time spatch --all-includes --sp-file contrib/coccinelle/unused.cocci builtin/log.c > warning: Can't find macro file: /usr/local/bin/lib/coccinelle/standard.h > warning: Can't find default iso file: /usr/local/bin/lib/coccinelle/standard.iso > HANDLING: builtin/log.c > Note: processing took 83.1s: builtin/log.c > > real 1m23.083s > user 1m22.983s If you remove the "done:" line in cmd_format_patch() buiiltin/log.c runs in ~200ms instead of ~40s for me. Perhaps we should be discussing removing or refactoring that one line of code instead? :) Removing coccinelle rules because we're seeing slowness somewhere seems particularly short-sighted to me. Maybe we do run into intractable problems somewhere with it being slow, and we'd also like to cater to more "interactive" use. But we shouldn't do that by removing rules until we get below some runtime limit, but rather by creating a "batch" category or something (just like we have "pending") now. Or, just actually look into why it's slow and fix those issues and/or report them upstream. There's nothing in unused.cocci that we either aren't running into elsewhere, or wouldn't run into if we had 10x the coccinelle rules we have now (which I think would be a good direction, we should rely on it more heavily). I've found that being able to have a ccache-like tool for "spatch"[1] solved almost all of the practical performance concerns I had with it. I.e. I can just run things in a batch, and usually any interactive use will hit things already in cache. To the extent it doesn't it's usually some pathological issue in spatch. > sys 0m0.033s > $ time spatch --all-includes --sp-file contrib/coccinelle/unused.cocci builtin/rebase.c > warning: Can't find macro file: /usr/local/bin/lib/coccinelle/standard.h > warning: Can't find default iso file: /usr/local/bin/lib/coccinelle/standard.iso > HANDLING: builtin/rebase.c > Note: processing took 83.2s: builtin/rebase.c > > real 1m23.223s > user 1m23.156s > sys 0m0.017s I didn't look at this one, but I assume it's some similar (and probably easily fixed) pathological issue. 1. https://lore.kernel.org/git/patch-5.5-ce4734e5d79-20220825T141212Z-avarab@xxxxxxxxx/
