| Message ID | f867512c-e5b2-6bca-2a37-2976f4c182bd@web.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | coccinelle: merge two rules from flex_alloc.cocci | expand |
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 >
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
>>> 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
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
> 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
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/
>> 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
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.
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) + );