Re: [PATCH v2] promisor-remote: fix xcalloc() argument order

["SZEDER Gábor" <szeder.dev@gmail.com>]

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