[PATCH] coccinelle: merge two rules from flex_alloc.cocci

[PATCH] coccinelle: merge two rules from flex_alloc.cocci

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 12 Nov 2019 16:30:14 +0100

This script contained two transformation rules for the semantic patch language
which used duplicate code.
Thus combine these rules by using a SmPL disjunction for the replacement
of two identifiers.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 contrib/coccinelle/flex_alloc.cocci | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/contrib/coccinelle/flex_alloc.cocci b/contrib/coccinelle/flex_alloc.cocci
index e9f7f6d861..1b4fa8f801 100644
--- a/contrib/coccinelle/flex_alloc.cocci
+++ b/contrib/coccinelle/flex_alloc.cocci
@@ -1,13 +1,14 @@
-@@
+@adjustment@
 expression str;
-identifier x, flexname;
-@@
-- FLEX_ALLOC_MEM(x, flexname, str, strlen(str));
-+ FLEX_ALLOC_STR(x, flexname, str);
-
-@@
-expression str;
-identifier x, ptrname;
-@@
-- FLEXPTR_ALLOC_MEM(x, ptrname, str, strlen(str));
-+ FLEXPTR_ALLOC_STR(x, ptrname, str);
+identifier x, name;
+@@
+(
+-FLEX_ALLOC_MEM
++FLEX_ALLOC_STR
+|
+-FLEXPTR_ALLOC_MEM
++FLEXPTR_ALLOC_STR
+)
+               (x, name, str
+-                           , strlen(str)
+               );
--
2.24.0

Re: [PATCH] coccinelle: merge two rules from flex_alloc.cocci

[Denton Liu]

Hi Markus,

Thanks for the contribution.

I see that you've sent many Coccinelle patches to the mailing list. It
might be better to send them all together as a single threaded patchset
so that reviewers will have an easier time finding all of them.

On Tue, Nov 12, 2019 at 04:34:34PM +0100, Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Tue, 12 Nov 2019 16:30:14 +0100
> 
> This script contained two transformation rules for the semantic patch language
> which used duplicate code.
> Thus combine these rules by using a SmPL disjunction for the replacement
> of two identifiers.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  contrib/coccinelle/flex_alloc.cocci | 25 +++++++++++++------------
>  1 file changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/contrib/coccinelle/flex_alloc.cocci b/contrib/coccinelle/flex_alloc.cocci
> index e9f7f6d861..1b4fa8f801 100644
> --- a/contrib/coccinelle/flex_alloc.cocci
> +++ b/contrib/coccinelle/flex_alloc.cocci
> @@ -1,13 +1,14 @@
> -@@
> +@adjustment@

None of our other cocci scripts have rulenames so I would drop the
rulename here. It also doesn't really help since its name is so generic.

I would also echo this for the other patches you've sent.

>  expression str;
> -identifier x, flexname;
> -@@
> -- FLEX_ALLOC_MEM(x, flexname, str, strlen(str));
> -+ FLEX_ALLOC_STR(x, flexname, str);
> -
> -@@
> -expression str;
> -identifier x, ptrname;
> -@@
> -- FLEXPTR_ALLOC_MEM(x, ptrname, str, strlen(str));
> -+ FLEXPTR_ALLOC_STR(x, ptrname, str);
> +identifier x, name;
> +@@
> +(
> +-FLEX_ALLOC_MEM
> ++FLEX_ALLOC_STR
> +|
> +-FLEXPTR_ALLOC_MEM
> ++FLEXPTR_ALLOC_STR
> +)
> +               (x, name, str
> +-                           , strlen(str)
> +               );

Small nitpick but to be inline with how the rest of our cocci scripts
are written, I'd write this as

	  (x, name, str
	- , strlen(str)
	  );

Thanks,

Denton

> --
> 2.24.0
> 

Re: [PATCH] coccinelle: merge two rules from flex_alloc.cocci

[Martin Ågren]

Hi Markus,

On Tue, 12 Nov 2019 at 19:02, Denton Liu <liu.denton@gmail.com> wrote:
> I see that you've sent many Coccinelle patches to the mailing list. It
> might be better to send them all together as a single threaded patchset
> so that reviewers will have an easier time finding all of them.

FWIW, I had the same thought.

> > This script contained two transformation rules for the semantic patch language
> > which used duplicate code.
> > Thus combine these rules by using a SmPL disjunction for the replacement
> > of two identifiers.

My knowledge of coccinelle and cocci rules is basically zero, but my
impression from this list is that running "make coccicheck" can be
expensive, both in terms of time and memory. Do these patches help there
in any way? Or could they hurt?

Martin

Re: coccinelle: merge two rules from flex_alloc.cocci

[Markus Elfring]

>>> This script contained two transformation rules for the semantic patch language
>>> which used duplicate code.
>>> Thus combine these rules by using a SmPL disjunction for the replacement
>>> of two identifiers.
>
> My knowledge of coccinelle and cocci rules is basically zero,

Would you like to change this situation eventually?


> but my impression from this list is that running "make coccicheck"
> can be expensive, both in terms of time and memory.

The desired source code analysis to detect possible software transformations
needs additional data processing resources.
It is usually hoped that corresponding efforts will help with development
approaches at other places.


> Do these patches help there in any way?

I hope so to some degree.

How much do you care to avoid code duplication?


> Or could they hurt?

I assume that you ask according to the presented change possibilities
for Git's SmPL scripts (and not only for “flex_alloc.cocci”).

Some changes usually contain the risk for undesirable effects.
Would you like to clarify each of them in more detail?

Regards,
Markus

Re: coccinelle: merge two rules from flex_alloc.cocci

[Martin Ågren]

On Wed, 13 Nov 2019 at 22:10, Markus Elfring <Markus.Elfring@web.de> wrote:
>
> >>> This script contained two transformation rules for the semantic patch language
> >>> which used duplicate code.
> >>> Thus combine these rules by using a SmPL disjunction for the replacement
> >>> of two identifiers.
> >
> > My knowledge of coccinelle and cocci rules is basically zero,
>
> Would you like to change this situation eventually?

Possibly, yeah, but I think the key word there is "eventually". ;-)
But maybe I'll learn something from this exchange.

> > but my impression from this list is that running "make coccicheck"
> > can be expensive, both in terms of time and memory.
>
> The desired source code analysis to detect possible software transformations
> needs additional data processing resources.
> It is usually hoped that corresponding efforts will help with development
> approaches at other places.

Right. So by that logic, if this patch doubles the memory usage and/or time
consumption of "make coccicheck", wouldn't this patch be affecting those
other activities at other places of the code negatively? ;-)

I'm not saying that this patch DOES affect the time/memory usage
negatively, and I'm not saying this patch IS a net negative. Definitely
not. It's just that considering, e.g., 960154b9c1 ("coccicheck:
optionally batch spatch invocations", 2019-05-06), time/memory
consumption -- and the balancing of the two -- seems to be an actual
real-world issue here, worth thinking about.

(That commit message mentions that processing all source files in one go
requires close to 2GB of memory. We've since started processing more
files.)

> > Do these patches help there in any way?
>
> I hope so to some degree.

If you could have some before/after numbers, that would be cool. If you
collect your patches into one series, you could at least do measurements
before/after the series.

Or if you could make some other sort of claim around "this shouldn't
affect this-or-that because so-and-so".

> How much do you care to avoid code duplication?

I tend to like it, everything else equal.

> > Or could they hurt?
>
> I assume that you ask according to the presented change possibilities
> for Git's SmPL scripts (and not only for “flex_alloc.cocci”).
>
> Some changes usually contain the risk for undesirable effects.
> Would you like to clarify each of them in more detail?

Do you mean whether I would like to clarify the risks I see, or do you
mean whether I would like you to clarify which you see? I've tried to
clarify the one I see -- based on passively observing cocci-related
patches floating around this list. If you see other potential risks,
feel free to mention them.

You seem to know lots more than I do about these things. I wouldn't be
surprised if you know more on this than most or all other participants
on this list, so feel free to share some of that in the commit messages
so that others can understand how you've reasoned.

Martin

Re: coccinelle: merge two rules from flex_alloc.cocci

[Markus Elfring]

> If you could have some before/after numbers, that would be cool.

Does any test infrastructure (or benchmarks) exist which you would trust for
corresponding comparisons of software run time characteristics?


> If you collect your patches into one series, you could at least do measurements
> before/after the series.

How do you think about to check possible improvements by each presented patch
according to affected SmPL scripts individually?


> Or if you could make some other sort of claim around "this shouldn't
> affect this-or-that because so-and-so".

I try to avoid such claims.
But I provided specific information in my patch descriptions.
Do you find any details reasonable there?


> Do you mean whether I would like to clarify the risks I see, or do you
> mean whether I would like you to clarify which you see?

Both. - The discussion will depend also on your change acceptance and desire
to extend development in the shown software design directions.


> I've tried to clarify the one I see -- based on passively observing cocci-related
> patches floating around this list.

Thanks for your interest.


> If you see other potential risks, feel free to mention them.

I would prefer a more optimistic view while my software development
experiences can influence this considerably.


> You seem to know lots more than I do about these things.

This can be. - But I hope that you can get further inspirations and ideas
if you would find any of the published development activities interesting enough.

Regards,
Markus

Re: coccinelle: merge two rules from flex_alloc.cocci

[SZEDER Gábor]

On Thu, Nov 14, 2019 at 09:15:47AM +0100, Markus Elfring wrote:
> > If you could have some before/after numbers, that would be cool.
> 
> Does any test infrastructure (or benchmarks) exist which you would trust for
> corresponding comparisons of software run time characteristics?

Yes, just run:

  make cocciclean
  time make contrib/coccinelle/flex_alloc.cocci.patch

before and after your changes, and include the timing results in the
commit message if there is a notable difference.  If it gets faster,
great!  If it gets slower, then update the commit message with a
convincing argument about why the change is worth the performance
penalty.

FWIW, I did just that with your "coccinelle: merge twelve rules from
object_id.cocci" patch [1], and the runtime went down from 2m48.610 to
2m34.395, a bit over 8% speedup. (with Ubuntu 16.04's Coccinelle
1.0.4)


[1] https://public-inbox.org/git/6c9962c0-67c1-e700-c145-793ce6498099@web.de/

Re: coccinelle: merge two rules from flex_alloc.cocci

[Markus Elfring]

>> Does any test infrastructure (or benchmarks) exist which you would trust for
>> corresponding comparisons of software run time characteristics?
>
> Yes, just run:
>
>   make cocciclean
>   time make contrib/coccinelle/flex_alloc.cocci.patch

Thanks for this suggestion.


> before and after your changes, and include the timing results in the
> commit message if there is a notable difference.

The measurements from my test system might not be representative enough.


> FWIW, I did just that with your "coccinelle: merge twelve rules from
> object_id.cocci" patch [1], and the runtime went down from 2m48.610 to
> 2m34.395, a bit over 8% speedup. (with Ubuntu 16.04's Coccinelle
> 1.0.4)
>
>
> [1] https://public-inbox.org/git/6c9962c0-67c1-e700-c145-793ce6498099@web.de/

These numbers seem to be nice.
Would a direct reply have been more helpful for the referenced patch
than the current subject?

Regards,
Markus

Re: coccinelle: merge two rules from flex_alloc.cocci

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

> On Thu, Nov 14, 2019 at 09:15:47AM +0100, Markus Elfring wrote:
>> > If you could have some before/after numbers, that would be cool.
>> 
>> Does any test infrastructure (or benchmarks) exist which you would trust for
>> corresponding comparisons of software run time characteristics?
>
> Yes, just run:
>
>   make cocciclean
>   time make contrib/coccinelle/flex_alloc.cocci.patch
>
> before and after your changes, and include the timing results in the
> commit message if there is a notable difference.  If it gets faster,
> great!  If it gets slower, then update the commit message with a
> convincing argument about why the change is worth the performance
> penalty.

Also, the contents of the *.patch file before and after the change
needs to match, but for that test to be meaningful, you'd need to
resurrect the problems we fixed with the help of the existing cocci
scripts (otherwise, the resulting *.patch file being empty does not
prove much---our source may now be too clean to demonstrate the
difference of the before/after rules).

Thanks.