Re: [PATCH v2] promisor-remote: fix xcalloc() argument order
- From
SZEDER Gábor <szeder.dev@gmail.com>
- Date
- Aug 24, 2022, 11:39 UTC
- Message-ID
- <20220824113901.GD1735@szeder.dev>
- In-Reply-To
- <YwXiKXONiTt+fIdi@coredump.intra.peff.net>
On Wed, Aug 24, 2022 at 04:32:41AM -0400, Jeff King wrote:
Show 15 quoted lines
> 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 usedSo 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);