On Wed, Aug 31 2022, SZEDER Gábor wrote:

On Wed, Aug 31, 2022 at 02:13:51PM +0200, Ævar Arnfjörð Bjarmason wrote:

quoted

On Wed, Aug 31 2022, SZEDER Gábor wrote:

quoted

[...]

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.

quoted

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.

quoted

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 :)

quoted

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.

quoted

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.