On Wed, Aug 31 2022, SZEDER Gábor wrote: > On Wed, Aug 31, 2022 at 02:13:51PM +0200, Ævar Arnfjörð Bjarmason wrote: >> >> On Wed, Aug 31 2022, SZEDER Gábor wrote: >>> [...] >> 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. If we're playing hypotheticals then if it hadn't been merged in the first place we'd have the UNUSED() macro all over the place on master now, and several *other* rules would probably be more broken (but I haven't looked into the cooci guts enough), we just wouldn't notice since "unused" is currently the only "look ahead" rule that fired & caught it :) >> 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 think we're just describing our workflows by proxy here. For me I generally take wasting a computer's time over a person's time any day. We should always be optimizing for saving people's time over CPUs, because the latter is relatively cheap. Granted unused.cocci isn't doing much in the grand scheme of things, but if you know it's there it's one more thing you can let your eyes wander over in patch review. You don't need to worry if some "struct strbuf" (or similar) is really being used, or is leftover boilerplate. Yes, it would be useful if coccicheck were currently fast enough to run as part of an edit-save-compile-check cycle, but like e.g. -fanalyzer it's much too slow for that for most people. So it gets run as some "batch' job. I think most people who use it in git.git are doing so by pushing to CI. There it doesn't really matter that it takes longer now, because as long as it's not slower than the longest running job it won't hold up the total CI runtime, which is in the ~1hr range anyway. So that's how I use it (when based off master). I also run it when I build my local git, but that's a "kick it off in the background and wait" type of operation. It runs tests, then other tests with other compilers, SANITIZE=address etc. etc. So I'm sorry if I tripped up some workflow for you with this change, but I do still think it's worth it. >> 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'. I did notice FWIW, but since I'm mainly looking at CI and other "batch" operations when looking at "coccicheck" I thought the trade-off was worth it.
