From: "SZEDER Gábor" <szeder.dev@gmail.com>
To: Jeff King <peff@peff.net>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH v2] promisor-remote: fix xcalloc() argument order
Date: Wed, 24 Aug 2022 13:39:01 +0200 [thread overview]
Message-ID: <20220824113901.GD1735@szeder.dev> (raw)
In-Reply-To: <YwXiKXONiTt+fIdi@coredump.intra.peff.net>
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);
next prev parent reply other threads:[~2022-08-24 11:39 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-08-22 21:34 [PATCH] promisor-remote: fix xcalloc() argument order SZEDER Gábor
2022-08-22 22:14 ` Junio C Hamano
2022-08-23 1:09 ` Junio C Hamano
2022-08-23 7:04 ` SZEDER Gábor
2022-08-23 9:21 ` SZEDER Gábor
2022-08-23 9:56 ` SZEDER Gábor
2022-08-23 9:57 ` [PATCH v2] " SZEDER Gábor
2022-08-24 8:32 ` Jeff King
2022-08-24 11:39 ` SZEDER Gábor [this message]
2022-08-25 10:40 ` Jeff King
2022-08-25 13:51 ` Ævar Arnfjörð Bjarmason
2022-08-24 15:58 ` Junio C Hamano
2022-08-24 17:33 ` SZEDER Gábor
2022-08-24 21:13 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20220824113901.GD1735@szeder.dev \
--to=szeder.dev@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=peff@peff.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).