threads / patch / 52243

patchcoccinelle: merge two rules from flex_alloc.cocci

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

## tl;dr

9 messages between Nov 12, 2019 and Nov 15, 2019. Diffs are folded; open one to read it.

replies: 8people: 5as markdown or json

Markus Elfring· Nov 12, 2019, 15:34 UTC · lore
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(-)
Show changes to contrib/coccinelle/flex_alloc.cocci +13 −13
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
Denton Liu· Nov 12, 2019, 17:59 UTC · re: Markus Elfring · lore

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

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:
Show 20 quoted lines
> 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.
Show 24 quoted lines
>  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
> 
Martin Ågren· Nov 13, 2019, 18:27 UTC · re: Denton Liu · lore

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

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
Markus Elfring· Nov 13, 2019, 21:10 UTC · re: Martin Ågren · lore

Re: coccinelle: merge two rules from flex_alloc.cocci

Show 6 quoted lines
>>> 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

Martin Ågren· Nov 14, 2019, 06:37 UTC · re: Markus Elfring · lore

Re: coccinelle: merge two rules from flex_alloc.cocci

On Wed, 13 Nov 2019 at 22:10, Markus Elfring <Markus.Elfring@web.de> wrote:
Show 9 quoted lines
>
> >>> 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.

Show 7 quoted lines
> > 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.
Show 7 quoted lines
> > 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
Markus Elfring· Nov 14, 2019, 08:15 UTC · re: Martin Ågren · lore

Re: coccinelle: merge two rules from flex_alloc.cocci

> 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

SZEDER Gábor· Nov 14, 2019, 16:35 UTC · re: Markus Elfring · lore

Re: coccinelle: merge two rules from flex_alloc.cocci

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/
Markus Elfring· Nov 14, 2019, 17:30 UTC · re: SZEDER Gábor · lore

Re: coccinelle: merge two rules from flex_alloc.cocci

Show 7 quoted lines
>> 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.
Show 7 quoted lines
> 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

Junio C Hamano· Nov 15, 2019, 05:13 UTC · re: SZEDER Gábor · lore

Re: coccinelle: merge two rules from flex_alloc.cocci

SZEDER Gábor <szeder.dev@gmail.com> writes:
Show 16 quoted lines
> 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.

← back to recent threads