On Wed, Aug 31, 2022 at 02:13:51PM +0200, Ævar Arnfjörð Bjarmason wrote: > > 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. It's not just slowness, it's drastic slowness. I'm looking at two "from scratch" 'make coccicheck' runs here, one with 'unused.cocci' taking 9m51s, one without taking 4m56s. So 'unused.cocci' effectively doubled the runtime, and wastes other developers' time and resources. I don't see anything wrong with removing a semantic patch that is as slow as 'unused.cocci' in its current form on our current codebase. We can always re-add it later, after those interested managed to figure out a way to address its slowness, and updated the semantic patch and/or the codebase accordingly. > Maybe we do run into intractable problems somewhere with it being slow, Looking at the runtimes I showed above, I think deeming it intractable is fully justified. > 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. IMO this should be the other way around: if applying a semantic patch is this slow, then first look into why it's slow, fix it, and only then submit it for merging. A semantic patch this slow shouldn't have been merged in the first place. > 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). Several developers have already stated that they might run 'make coccicheck' more often if it weren't so slow. I think we must keep this in mind when adding new semantic patches, and should aim for a good return of investment between the usefulness of the semantic patch and its overhead. 'unused.cocci' doesn't seem to strike a good balance here. I doubt that I would ever run 'make coccicheck' if we had 10x as many semantic patches. > 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. Well, perhaps that's why you didn't notice just how slow 'unused.cocci' can be... :) Please don't forget about the runtime of a default "from scratch" 'make coccicheck'. > 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/
