On Wed, Aug 24, 2022 at 04:32:41AM -0400, Jeff King wrote:
> On Tue, Aug 23, 2022 at 11:57:33AM +0200, SZEDER Gábor wrote:
>
> > Pass the number of elements first and their size second, as expected
> > by xcalloc().
> >
> > Patch generated with:
> >
> > make SPATCH_FLAGS=--recursive-includes contrib/coccinelle/xcalloc.cocci.patch
>
> Thanks for digging here. I think it probably explains a lot of "wait,
> why did this result change" head-scratching I did back when we started
> batching a few years ago.
>
> Is there any reason we wouldn't want --recursive-includes to be added to
> the default SPATCH_FLAGS?
I ran some runtime measurements yesterday, and there is a considerable
runtime increase in the unbatched case:
The tables below show the runtimes of applying each of our semantic
patches separately with Coccinelle v1.1.1, with the '--all-includes'
or '--recursive-includes' flags and with batch sizes 1 (no batching)
or 32, i.e.:
time make SPATCH_FLAGS=$flag SPATCH_BATCH_SIZE=$size $cocci.patch
SPATCH_BATCH_SIZE=1:
semantic patch | all | recursive |
-----------------+-----------+-----------+--------
array | 69.90s | 140.12s | +100%
commit | 94.44s | 223.63s | +137%
equals-null | 86.93s | 205.40s | +136%
flex_alloc | 11.32s | 16.45s | +45%
free | 70.47s | 159.75s | +127%
hashmap | 83.48s | 199.70s | +139%
object_id | 107.83s | 241.69s | +124%
preincr | 79.33s | 202.98s | +156%
qsort | 16.20s | 33.86s | +109%
strbuf | 60.54s | 129.93s | +115%
swap | 81.70s | 200.75s | +146%
unused | 499.19s | 626.35s | +25%
xcalloc | 26.71s | 57.63s | +116%
xopen | 30.92s | 59.26s | +92%
xstrdup_or_null | 5.05s | 6.94s | +37%
-----------------+-----------+-----------+--------
all | 1324s | 2504s | +89%
SPATCH_BATCH_SIZE=32:
semantic patch | all | recursive |
-----------------+-----------+-----------+--------
array | 43.81s | 52.83s | +21%
commit | 50.16s | 52.76s | +5%
equals-null | 47.77s | 50.86s | +6%
flex_alloc | 41.00s | 43.64s | +6%
free | 43.12s | 46.68s | +8%
hashmap | 42.76s | 46.12s | +8%
object_id | 56.17s | 60.00s | +7%
preincr | 39.82s | 42.57s | +7%
qsort | 39.48s | 53.09s | +34%
strbuf | 51.27s | 49.38s | -4%
swap | 41.93s | 58.17s | +39%
unused | 440.86s | 445.47s | +1%
xcalloc | 39.90s | 42.22s | +6%
xopen | 40.26s | 43.19s | +7%
xstrdup_or_null | 39.14s | 41.72s | +7%
-----------------+-----------+-----------+--------
all | 1057s | 1129s | +7%
I don't have meaningful numbers about the impact of
'--recursive-includes' on the runtime of a parallel 'make -j<N>
coccicheck', because they're severely skewed by 'unused.cocci' and
'make's scheduler: applying 'unused.cocci' takes 5-10x as long as
other semantic patches (accounting for about 38-42% of the total
sequential runtime), and 'make' on my system usually schedules it near
the end, meaning that for a significant part there is no parallel
processing at all.
So I'm not sure about making '--recursive-includes' the default, at
least as long as we don't batch by default. However, we could still
use this flag in CI...
> I suspect we'd still want to leave --all-includes there to get as much
> type information as possible. If I understand correctly, the two are
> orthogonal (one is "follow includes of includes" and the other is
> "follow system includes in angle brackets").
'spatch --help' tells me:
--recursive-includes causes all available include files, both
those included in the C file(s) and those
included in header files, to be used
--all-includes causes all available include files included
in the C file(s) to be used
So I think the difference is not about system includes, but rather
about "consider includes of includes" vs. "consider only direct
includes in the C files", so it seems '--recursive-includes' is a
superset of '--all-includes'. Oh, and let's not forget the rather
surprising behavior that if spatch processes multiple C files at once,
then for each of the C files it considers all includes from all C
files; this explains why '--all-includes promisor-remote.c config.c'
works, because 'config.c' does include 'repository.h', supplying the
necessary type information to catch the previously missed
transformation in 'promisor-remote.c'.
These descriptions don't make it clear (to me) whether the two options
are orthogonal, exclusive, or something else. However, trying out various
combinations indicates that "last one wins", e.g.:
$ spatch --recursive-includes --all-includes --sp-file contrib/coccinelle/xcalloc.cocci --patch . promisor-remote.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: promisor-remote.c
$ spatch --all-includes --recursive-includes --sp-file contrib/coccinelle/xcalloc.cocci --patch . promisor-remote.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: promisor-remote.c
diff =
diff -u -p a/promisor-remote.c b/promisor-remote.c
--- a/promisor-remote.c
+++ b/promisor-remote.c
@@ -146,7 +146,7 @@ static void promisor_remote_init(struct
if (r->promisor_remote_config)
return;
config = r->promisor_remote_config =
- xcalloc(sizeof(*r->promisor_remote_config), 1);
+ xcalloc(1, sizeof(*r->promisor_remote_config));
config->promisors_tail = &config->promisors;
repo_config(r, promisor_remote_config, config);
