{"thread":{"id":"49469","subject":"[PATCH] coccicheck: process every source file at once","startedAt":"2018-10-02T19:16:56Z","lastAt":"2018-10-03T17:55:08Z","messageCount":11,"participants":["Jacob Keller","Jeff King","SZEDER Gábor","Stefan Beller"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"359451","messageId":"20181002191642.21504-1-jacob.e.keller@intel.com","threadId":"49469","inReplyTo":null,"subject":"[PATCH] coccicheck: process every source file at once","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2018-10-02T19:16:42Z","receivedAt":"2018-10-02T19:16:56Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller <jacob.keller@gmail.com>\n\nmake coccicheck is used in order to apply coccinelle semantic patches,\nand see if any of the transformations found within contrib/coccinelle/\ncan be applied to the current code base.\n\nPass every file to a single invocation of spatch, instead of running\nspatch once per source file.\n\nThis reduces the time required to run make coccicheck by a significant\namount of time:\n\nPrior timing of make coccicheck\n  real    6m14.090s\n  user    25m2.606s\n  sys     1m22.919s\n\nNew timing of make coccicheck\n  real    1m36.580s\n  user    7m55.933s\n  sys     0m18.219s\n\nThis is nearly a 4x decrease in the time required to run make\ncoccicheck. This is due to the overhead of restarting spatch for every\nfile. By processing all files at once, we can amortize this startup cost\nacross the total number of files, rather than paying it once per file.\n\nSigned-off-by: Jacob Keller <jacob.keller@gmail.com>\n---\n Makefile | 6 ++----\n 1 file changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex df1df9db78da..b9947f3f51ec 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2715,10 +2715,8 @@ endif\n %.cocci.patch: %.cocci $(COCCI_SOURCES)\n \t@echo '    ' SPATCH $<; \\\n \tret=0; \\\n-\tfor f in $(COCCI_SOURCES); do \\\n-\t\t$(SPATCH) --sp-file $< $$f $(SPATCH_FLAGS) || \\\n-\t\t\t{ ret=$$?; break; }; \\\n-\tdone >$@+ 2>$@.log; \\\n+\t( $(SPATCH) --sp-file $< $(COCCI_SOURCES) $(SPATCH_FLAGS) || \\\n+\t\t{ ret=$$?; }; ) >$@+ 2>$@.log; \\\n \tif test $$ret != 0; \\\n \tthen \\\n \t\tcat $@.log; \\\n-- \n2.18.0.219.gaf81d287a9da\n\n"},{"id":"359456","messageId":"20181002195519.GB2014@sigill.intra.peff.net","threadId":"49469","inReplyTo":"20181002191642.21504-1-jacob.e.keller@intel.com","subject":"Re: [PATCH] coccicheck: process every source file at once","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-02T19:55:19Z","receivedAt":"2018-10-02T19:55:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 02, 2018 at 12:16:42PM -0700, Jacob Keller wrote:\n\n> make coccicheck is used in order to apply coccinelle semantic patches,\n> and see if any of the transformations found within contrib/coccinelle/\n> can be applied to the current code base.\n> \n> Pass every file to a single invocation of spatch, instead of running\n> spatch once per source file.\n> \n> This reduces the time required to run make coccicheck by a significant\n> amount of time:\n> \n> Prior timing of make coccicheck\n>   real    6m14.090s\n>   user    25m2.606s\n>   sys     1m22.919s\n> \n> New timing of make coccicheck\n>   real    1m36.580s\n>   user    7m55.933s\n>   sys     0m18.219s\n\nYay! This is a nice result.\n\nIt's also one of the things that Julia suggested in an earlier thread.\nOne thing I wasn't quite sure about after digging into the various\nversions (1.0.4 on Debian stable/unstable, 1.0.6 in experimental, and\npre-1.0.7 at the bleeding edge) was whether the old versions would be\nsimilarly helped (or work at all).\n\nI just replicated your results with 1.0.4.deb-3+b2 from Debian stable.\nIt's possible there are older versions floating around, but for\nsomething developer-only like this, I think \"in Debian stable\" is a\nreasonable enough cutoff.\n\n> This is nearly a 4x decrease in the time required to run make\n> coccicheck. This is due to the overhead of restarting spatch for every\n> file. By processing all files at once, we can amortize this startup cost\n> across the total number of files, rather than paying it once per file.\n\nOne minor side effect is that we lose the opportunity to parallelize\nquite as much. However, I think the reduction in total CPU makes it well\nworth that (and we still have 8 cocci files and growing, which should\nkeep at least 8 cores busy).\n\nI think recent versions of Coccinelle will actually parallelize\ninternally, too, but my 1.0.4 doesn't seem to. That's probably what I\nwas thinking of earlier (but this is still a win even without that).\n\nIt looks like this also fixes a problem I ran into when doing the oideq\nwork, which is that the patch for a header file would get shown multiple\ntimes (once for each file that includes it). So this is faster _and_\nmore correct. Double-yay.\n\n> diff --git a/Makefile b/Makefile\n> index df1df9db78da..b9947f3f51ec 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -2715,10 +2715,8 @@ endif\n>  %.cocci.patch: %.cocci $(COCCI_SOURCES)\n>  \t@echo '    ' SPATCH $<; \\\n>  \tret=0; \\\n> -\tfor f in $(COCCI_SOURCES); do \\\n> -\t\t$(SPATCH) --sp-file $< $$f $(SPATCH_FLAGS) || \\\n> -\t\t\t{ ret=$$?; break; }; \\\n> -\tdone >$@+ 2>$@.log; \\\n> +\t( $(SPATCH) --sp-file $< $(COCCI_SOURCES) $(SPATCH_FLAGS) || \\\n> +\t\t{ ret=$$?; }; ) >$@+ 2>$@.log; \\\n\nThis looks pretty straight-forward. I wondered if we could get rid of\nthe \"ret\" handling, since we don't need to pass the error back out of\nthe loop. But it's also used for the \"show the log only on error\" logic\nbelow:\n\n>  \tif test $$ret != 0; \\\n>  \tthen \\\n>  \t\tcat $@.log; \\\n\nThe subshell could be a {}, though, I think, but it's not that big a\ndeal either way.\n\n-Peff\n"},{"id":"359457","messageId":"CA+P7+xrjpEe_3_mAwZ73O2JP2Pd518OFnSf+gfmbTJW7A3Q=Nw@mail.gmail.com","threadId":"49469","inReplyTo":"20181002195519.GB2014@sigill.intra.peff.net","subject":"Re: [PATCH] coccicheck: process every source file at once","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2018-10-02T20:00:21Z","receivedAt":"2018-10-02T20:00:37Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Tue, Oct 2, 2018 at 12:55 PM Jeff King <peff@peff.net> wrote:\n>\n> On Tue, Oct 02, 2018 at 12:16:42PM -0700, Jacob Keller wrote:\n>\n> > make coccicheck is used in order to apply coccinelle semantic patches,\n> > and see if any of the transformations found within contrib/coccinelle/\n> > can be applied to the current code base.\n> >\n> > Pass every file to a single invocation of spatch, instead of running\n> > spatch once per source file.\n> >\n> > This reduces the time required to run make coccicheck by a significant\n> > amount of time:\n> >\n> > Prior timing of make coccicheck\n> >   real    6m14.090s\n> >   user    25m2.606s\n> >   sys     1m22.919s\n> >\n> > New timing of make coccicheck\n> >   real    1m36.580s\n> >   user    7m55.933s\n> >   sys     0m18.219s\n>\n> Yay! This is a nice result.\n>\n> It's also one of the things that Julia suggested in an earlier thread.\n> One thing I wasn't quite sure about after digging into the various\n> versions (1.0.4 on Debian stable/unstable, 1.0.6 in experimental, and\n> pre-1.0.7 at the bleeding edge) was whether the old versions would be\n> similarly helped (or work at all).\n>\n> I just replicated your results with 1.0.4.deb-3+b2 from Debian stable.\n> It's possible there are older versions floating around, but for\n> something developer-only like this, I think \"in Debian stable\" is a\n> reasonable enough cutoff.\n>\n\nGood. I hadn't checked back too far, but I know support for multiple\nfiles has existed since quite a while.\n\n> > This is nearly a 4x decrease in the time required to run make\n> > coccicheck. This is due to the overhead of restarting spatch for every\n> > file. By processing all files at once, we can amortize this startup cost\n> > across the total number of files, rather than paying it once per file.\n>\n> One minor side effect is that we lose the opportunity to parallelize\n> quite as much. However, I think the reduction in total CPU makes it well\n> worth that (and we still have 8 cocci files and growing, which should\n> keep at least 8 cores busy).\n>\n\nI don't think we do any less than previously, because we are doing\neach file in a for-loop, so those would all be serial anyways.\n\n> I think recent versions of Coccinelle will actually parallelize\n> internally, too, but my 1.0.4 doesn't seem to. That's probably what I\n> was thinking of earlier (but this is still a win even without that).\n>\n> It looks like this also fixes a problem I ran into when doing the oideq\n> work, which is that the patch for a header file would get shown multiple\n> times (once for each file that includes it). So this is faster _and_\n> more correct. Double-yay.\n>\n\nWoot :D\n\n> > diff --git a/Makefile b/Makefile\n> > index df1df9db78da..b9947f3f51ec 100644\n> > --- a/Makefile\n> > +++ b/Makefile\n> > @@ -2715,10 +2715,8 @@ endif\n> >  %.cocci.patch: %.cocci $(COCCI_SOURCES)\n> >       @echo '    ' SPATCH $<; \\\n> >       ret=0; \\\n> > -     for f in $(COCCI_SOURCES); do \\\n> > -             $(SPATCH) --sp-file $< $$f $(SPATCH_FLAGS) || \\\n> > -                     { ret=$$?; break; }; \\\n> > -     done >$@+ 2>$@.log; \\\n> > +     ( $(SPATCH) --sp-file $< $(COCCI_SOURCES) $(SPATCH_FLAGS) || \\\n> > +             { ret=$$?; }; ) >$@+ 2>$@.log; \\\n>\n> This looks pretty straight-forward. I wondered if we could get rid of\n> the \"ret\" handling, since we don't need to pass the error back out of\n> the loop. But it's also used for the \"show the log only on error\" logic\n> below:\n>\n\nWe could probably get rid of it by doing the spatch without an ||, and\nthen just save $?.\n\n> >       if test $$ret != 0; \\\n> >       then \\\n> >               cat $@.log; \\\n>\n> The subshell could be a {}, though, I think, but it's not that big a\n> deal either way.\n>\n> -Peff\n"},{"id":"359464","messageId":"20181002203124.GC2014@sigill.intra.peff.net","threadId":"49469","inReplyTo":"CA+P7+xrjpEe_3_mAwZ73O2JP2Pd518OFnSf+gfmbTJW7A3Q=Nw@mail.gmail.com","subject":"Re: [PATCH] coccicheck: process every source file at once","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-02T20:31:24Z","receivedAt":"2018-10-02T20:31:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 02, 2018 at 01:00:21PM -0700, Jacob Keller wrote:\n\n> > > This is nearly a 4x decrease in the time required to run make\n> > > coccicheck. This is due to the overhead of restarting spatch for every\n> > > file. By processing all files at once, we can amortize this startup cost\n> > > across the total number of files, rather than paying it once per file.\n> >\n> > One minor side effect is that we lose the opportunity to parallelize\n> > quite as much. However, I think the reduction in total CPU makes it well\n> > worth that (and we still have 8 cocci files and growing, which should\n> > keep at least 8 cores busy).\n> >\n> \n> I don't think we do any less than previously, because we are doing\n> each file in a for-loop, so those would all be serial anyways.\n\nYeah, sorry to be unclear. This is a strict improvement on what was\nthere before. We had floated some patches to do the full NxM\nparallelization, but I think this path is better because of the\nreduction in actual CPU used (and if more recent versions of spatch can\nparallelize internally, we'll eventually get the best of both worlds).\n\n> > > diff --git a/Makefile b/Makefile\n> > > index df1df9db78da..b9947f3f51ec 100644\n> > > --- a/Makefile\n> > > +++ b/Makefile\n> > > @@ -2715,10 +2715,8 @@ endif\n> > >  %.cocci.patch: %.cocci $(COCCI_SOURCES)\n> > >       @echo '    ' SPATCH $<; \\\n> > >       ret=0; \\\n> > > -     for f in $(COCCI_SOURCES); do \\\n> > > -             $(SPATCH) --sp-file $< $$f $(SPATCH_FLAGS) || \\\n> > > -                     { ret=$$?; break; }; \\\n> > > -     done >$@+ 2>$@.log; \\\n> > > +     ( $(SPATCH) --sp-file $< $(COCCI_SOURCES) $(SPATCH_FLAGS) || \\\n> > > +             { ret=$$?; }; ) >$@+ 2>$@.log; \\\n> >\n> > This looks pretty straight-forward. I wondered if we could get rid of\n> > the \"ret\" handling, since we don't need to pass the error back out of\n> > the loop. But it's also used for the \"show the log only on error\" logic\n> > below:\n> >\n> \n> We could probably get rid of it by doing the spatch without an ||, and\n> then just save $?.\n\nActually, I guess we do not need to save $? at all, since we have only a\nsingle process to care about. So even simpler:\n\n  spatch ... 2>$@+ 2>$@.log ||\n  {\n\tcat $@.log\n\texit 1\n  }\n  # if we get here, we were successful\n  mv $@+ $@ ;# etc\n\nwould work. That's missing all the Makefile=required backslashes and\nsemicolons, of course. ;)\n\n-Peff\n"},{"id":"359467","messageId":"CA+P7+xqxov6-+t6ixuVxUvi=yfjDuxsa8poGL0TrUrWoj3NUNA@mail.gmail.com","threadId":"49469","inReplyTo":"20181002203124.GC2014@sigill.intra.peff.net","subject":"Re: [PATCH] coccicheck: process every source file at once","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2018-10-02T20:58:10Z","receivedAt":"2018-10-02T20:58:24Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Tue, Oct 2, 2018 at 1:31 PM Jeff King <peff@peff.net> wrote:\n> Actually, I guess we do not need to save $? at all, since we have only a\n> single process to care about. So even simpler:\n>\n>   spatch ... 2>$@+ 2>$@.log ||\n>   {\n>         cat $@.log\n>         exit 1\n>   }\n>   # if we get here, we were successful\n>   mv $@+ $@ ;# etc\n>\n> would work. That's missing all the Makefile=required backslashes and\n> semicolons, of course. ;)\n>\n\nI opted to drop to just save the return, immediately after calling.\nIt's a bit less code change, and I think the result is as clear as the\nabove would be. This way we do drop the subshell, not that it matters\nmuch in the end...\n\nThanks,\nJake\n\n> -Peff\n"},{"id":"359470","messageId":"20181002210821.GA27904@sigill.intra.peff.net","threadId":"49469","inReplyTo":"CA+P7+xqxov6-+t6ixuVxUvi=yfjDuxsa8poGL0TrUrWoj3NUNA@mail.gmail.com","subject":"Re: [PATCH] coccicheck: process every source file at once","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-02T21:08:21Z","receivedAt":"2018-10-02T21:08:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 02, 2018 at 01:58:10PM -0700, Jacob Keller wrote:\n\n> On Tue, Oct 2, 2018 at 1:31 PM Jeff King <peff@peff.net> wrote:\n> > Actually, I guess we do not need to save $? at all, since we have only a\n> > single process to care about. So even simpler:\n> >\n> >   spatch ... 2>$@+ 2>$@.log ||\n> >   {\n> >         cat $@.log\n> >         exit 1\n> >   }\n> >   # if we get here, we were successful\n> >   mv $@+ $@ ;# etc\n> >\n> > would work. That's missing all the Makefile=required backslashes and\n> > semicolons, of course. ;)\n> >\n> \n> I opted to drop to just save the return, immediately after calling.\n> It's a bit less code change, and I think the result is as clear as the\n> above would be. This way we do drop the subshell, not that it matters\n> much in the end...\n\nYeah. To be clear, I'm fine with any of the versions discussed in this\nthread. Thanks for working on this!\n\n-Peff\n"},{"id":"359506","messageId":"20181003101658.GM23446@localhost","threadId":"49469","inReplyTo":"20181002195519.GB2014@sigill.intra.peff.net","subject":"Re: [PATCH] coccicheck: process every source file at once","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-10-03T10:16:58Z","receivedAt":"2018-10-03T10:17:05Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Oct 02, 2018 at 03:55:19PM -0400, Jeff King wrote:\n> On Tue, Oct 02, 2018 at 12:16:42PM -0700, Jacob Keller wrote:\n> \n> > make coccicheck is used in order to apply coccinelle semantic patches,\n> > and see if any of the transformations found within contrib/coccinelle/\n> > can be applied to the current code base.\n> > \n> > Pass every file to a single invocation of spatch, instead of running\n> > spatch once per source file.\n> > \n> > This reduces the time required to run make coccicheck by a significant\n> > amount of time:\n> > \n> > Prior timing of make coccicheck\n> >   real    6m14.090s\n> >   user    25m2.606s\n> >   sys     1m22.919s\n> > \n> > New timing of make coccicheck\n> >   real    1m36.580s\n> >   user    7m55.933s\n> >   sys     0m18.219s\n> \n> Yay! This is a nice result.\n> \n> It's also one of the things that Julia suggested in an earlier thread.\n> One thing I wasn't quite sure about after digging into the various\n> versions (1.0.4 on Debian stable/unstable, 1.0.6 in experimental, and\n> pre-1.0.7 at the bleeding edge) was whether the old versions would be\n> similarly helped (or work at all).\n> \n> I just replicated your results with 1.0.4.deb-3+b2 from Debian stable.\n> It's possible there are older versions floating around, but for\n> something developer-only like this, I think \"in Debian stable\" is a\n> reasonable enough cutoff.\n\nLinux build jobs on Travis CI run Ubuntu Trusty 14.04 LTS, and\ntherefore our static analysis build job still runs 1.0.0~rc19.deb-3.\n\nThis patch appears to work fine with that version, too, though note\nthat I also changed the build job to don't run two parallel jobs; for\nthe reason see below.\n\n> > This is nearly a 4x decrease in the time required to run make\n> > coccicheck. This is due to the overhead of restarting spatch for every\n> > file. By processing all files at once, we can amortize this startup cost\n> > across the total number of files, rather than paying it once per file.\n> \n> One minor side effect is that we lose the opportunity to parallelize\n> quite as much. However, I think the reduction in total CPU makes it well\n> worth that (and we still have 8 cocci files and growing, which should\n> keep at least 8 cores busy).\n\nOne major side effect, however, is the drastically increased memory\nusage resulting from processing all files at once.  With your patch on\ntop of current master:\n\n  $ for cocci in contrib/coccinelle/*.cocci ; do command time -f 'max RSS: %MkB' make ${cocci}.patch ; done\n       SPATCH contrib/coccinelle/array.cocci\n  max RSS: 1537068kB\n       SPATCH contrib/coccinelle/commit.cocci\n  max RSS: 1510920kB\n       SPATCH contrib/coccinelle/free.cocci\n  max RSS: 1393712kB\n       SPATCH contrib/coccinelle/object_id.cocci\n       SPATCH result: contrib/coccinelle/object_id.cocci.patch\n  max RSS: 1831700kB\n       SPATCH contrib/coccinelle/qsort.cocci\n  max RSS: 1384960kB\n       SPATCH contrib/coccinelle/strbuf.cocci\n  max RSS: 1395800kB\n       SPATCH contrib/coccinelle/swap.cocci\n  max RSS: 1393620kB\n       SPATCH contrib/coccinelle/xstrdup_or_null.cocci\n  max RSS: 1371716kB\n\nWithout your patch the max RSS lingers around 87048kB - 101980kB,\nmeaning ~15x - 18x increase\n\nThis might cause quite the surprise to developers who are used to run\n'make -jN coccicheck'; my tiny laptop came to a grinding halt with\n-j4.\n\nI think this side effect should be mentioned in the commit message.\n\nFurthermore, the above mentioned static analysis build job on Travis\nCI runs two parallel jobs.  Perhaps we should be considerate and\nreduce that to a single job, even if that means that he build job will\nrun longer.\n\n> I think recent versions of Coccinelle will actually parallelize\n> internally, too, but my 1.0.4 doesn't seem to. That's probably what I\n> was thinking of earlier (but this is still a win even without that).\n\nI think Coccinelle parallelizes only when invoked with -j <N>, but\nthat option is not documented in 1.0.4.  I wrote \"documented\" instead\nof \"present\", because e.g.:\n\n  $ spatch --sp-file contrib/coccinelle/swap.cocci -j 2 a*.c\n\ndoesn't abort with \"unknown option\" and usage, but runs for a bit and\nthen outputs:\n\n  init_defs_builtins: /usr/lib/coccinelle/standard.h\n  HANDLING: abspath.c advice.c alias.c alloc.c apply.c archive.c archive-tar.c archive-zip.c argv-array.c attr.c\n  Fatal error: exception Sys_error(\"swap: No such file or directory\")\n\nWithout '-j 2' the command runs just fine.  I don't know what to make\nof it.\n\n> It looks like this also fixes a problem I ran into when doing the oideq\n> work, which is that the patch for a header file would get shown multiple\n> times (once for each file that includes it). So this is faster _and_\n> more correct. Double-yay.\n\nIn that case I used to apply the change to the header first, and then\napply the semantic patch one more time.\n\n"},{"id":"359531","messageId":"20181003150522.GQ23446@localhost","threadId":"49469","inReplyTo":"20181003101658.GM23446@localhost","subject":"Re: [PATCH] coccicheck: process every source file at once","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-10-03T15:05:22Z","receivedAt":"2018-10-03T15:05:34Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Wed, Oct 03, 2018 at 12:16:58PM +0200, SZEDER Gábor wrote:\n> On Tue, Oct 02, 2018 at 03:55:19PM -0400, Jeff King wrote:\n> > On Tue, Oct 02, 2018 at 12:16:42PM -0700, Jacob Keller wrote:\n> > \n> > > make coccicheck is used in order to apply coccinelle semantic patches,\n> > > and see if any of the transformations found within contrib/coccinelle/\n> > > can be applied to the current code base.\n> > > \n> > > Pass every file to a single invocation of spatch, instead of running\n> > > spatch once per source file.\n> > > \n> > > This reduces the time required to run make coccicheck by a significant\n> > > amount of time:\n> > > \n> > > Prior timing of make coccicheck\n> > >   real    6m14.090s\n> > >   user    25m2.606s\n> > >   sys     1m22.919s\n> > > \n> > > New timing of make coccicheck\n> > >   real    1m36.580s\n> > >   user    7m55.933s\n> > >   sys     0m18.219s\n> > \n> > Yay! This is a nice result.\n> > \n> > It's also one of the things that Julia suggested in an earlier thread.\n> > One thing I wasn't quite sure about after digging into the various\n> > versions (1.0.4 on Debian stable/unstable, 1.0.6 in experimental, and\n> > pre-1.0.7 at the bleeding edge) was whether the old versions would be\n> > similarly helped (or work at all).\n> > \n> > I just replicated your results with 1.0.4.deb-3+b2 from Debian stable.\n> > It's possible there are older versions floating around, but for\n> > something developer-only like this, I think \"in Debian stable\" is a\n> > reasonable enough cutoff.\n> \n> Linux build jobs on Travis CI run Ubuntu Trusty 14.04 LTS, and\n> therefore our static analysis build job still runs 1.0.0~rc19.deb-3.\n> \n> This patch appears to work fine with that version, too,\n\nIn fact, it works finer than ever, because running 1.0.0 with this\npatch on Travis CI notices a possible memmove() -> MOVE_ARRAY()\nconversion:\n\n  https://travis-ci.org/szeder/git/jobs/436542684#L576\n\nSurprisingly, running 1.0.0 without this patch, or running 1.0.4\nlocally either with or without this patch doesn't notice that\nmemmove() call.  Presumably that's why Jonathan could kind-of \"revert\"\nmy conversion from f919ffebed (Use MOVE_ARRAY, 2018-01-22) in his\n6a1a79fd14 (object: move grafts to object parser, 2018-05-15) without\nanyone noticing.\n\n"},{"id":"359536","messageId":"CA+P7+xoos=7WAn+L1W=_OrY6+_WvrptjhrGFPvhArq=WG0gi5A@mail.gmail.com","threadId":"49469","inReplyTo":"20181003101658.GM23446@localhost","subject":"Re: [PATCH] coccicheck: process every source file at once","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2018-10-03T15:51:54Z","receivedAt":"2018-10-03T15:52:09Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Wed, Oct 3, 2018 at 3:17 AM SZEDER Gábor <szeder.dev@gmail.com> wrote:\n>\n> On Tue, Oct 02, 2018 at 03:55:19PM -0400, Jeff King wrote:\n> > On Tue, Oct 02, 2018 at 12:16:42PM -0700, Jacob Keller wrote:\n> >\n> > > make coccicheck is used in order to apply coccinelle semantic patches,\n> > > and see if any of the transformations found within contrib/coccinelle/\n> > > can be applied to the current code base.\n> > >\n> > > Pass every file to a single invocation of spatch, instead of running\n> > > spatch once per source file.\n> > >\n> > > This reduces the time required to run make coccicheck by a significant\n> > > amount of time:\n> > >\n> > > Prior timing of make coccicheck\n> > >   real    6m14.090s\n> > >   user    25m2.606s\n> > >   sys     1m22.919s\n> > >\n> > > New timing of make coccicheck\n> > >   real    1m36.580s\n> > >   user    7m55.933s\n> > >   sys     0m18.219s\n> >\n> > Yay! This is a nice result.\n> >\n> > It's also one of the things that Julia suggested in an earlier thread.\n> > One thing I wasn't quite sure about after digging into the various\n> > versions (1.0.4 on Debian stable/unstable, 1.0.6 in experimental, and\n> > pre-1.0.7 at the bleeding edge) was whether the old versions would be\n> > similarly helped (or work at all).\n> >\n> > I just replicated your results with 1.0.4.deb-3+b2 from Debian stable.\n> > It's possible there are older versions floating around, but for\n> > something developer-only like this, I think \"in Debian stable\" is a\n> > reasonable enough cutoff.\n>\n> Linux build jobs on Travis CI run Ubuntu Trusty 14.04 LTS, and\n> therefore our static analysis build job still runs 1.0.0~rc19.deb-3.\n>\n> This patch appears to work fine with that version, too, though note\n> that I also changed the build job to don't run two parallel jobs; for\n> the reason see below.\n>\n> > > This is nearly a 4x decrease in the time required to run make\n> > > coccicheck. This is due to the overhead of restarting spatch for every\n> > > file. By processing all files at once, we can amortize this startup cost\n> > > across the total number of files, rather than paying it once per file.\n> >\n> > One minor side effect is that we lose the opportunity to parallelize\n> > quite as much. However, I think the reduction in total CPU makes it well\n> > worth that (and we still have 8 cocci files and growing, which should\n> > keep at least 8 cores busy).\n>\n> One major side effect, however, is the drastically increased memory\n> usage resulting from processing all files at once.  With your patch on\n> top of current master:\n>\n>   $ for cocci in contrib/coccinelle/*.cocci ; do command time -f 'max RSS: %MkB' make ${cocci}.patch ; done\n>        SPATCH contrib/coccinelle/array.cocci\n>   max RSS: 1537068kB\n>        SPATCH contrib/coccinelle/commit.cocci\n>   max RSS: 1510920kB\n>        SPATCH contrib/coccinelle/free.cocci\n>   max RSS: 1393712kB\n>        SPATCH contrib/coccinelle/object_id.cocci\n>        SPATCH result: contrib/coccinelle/object_id.cocci.patch\n>   max RSS: 1831700kB\n>        SPATCH contrib/coccinelle/qsort.cocci\n>   max RSS: 1384960kB\n>        SPATCH contrib/coccinelle/strbuf.cocci\n>   max RSS: 1395800kB\n>        SPATCH contrib/coccinelle/swap.cocci\n>   max RSS: 1393620kB\n>        SPATCH contrib/coccinelle/xstrdup_or_null.cocci\n>   max RSS: 1371716kB\n>\n> Without your patch the max RSS lingers around 87048kB - 101980kB,\n> meaning ~15x - 18x increase\n>\n\nQuite a significant increase.\n\n\n> This might cause quite the surprise to developers who are used to run\n> 'make -jN coccicheck'; my tiny laptop came to a grinding halt with\n> -j4.\n>\n> I think this side effect should be mentioned in the commit message.\n>\n\nYes I agree. I hadn't considered the memory problem.\n\nThanks,\nJake\n"},{"id":"359537","messageId":"CA+P7+xpB5Pqj7Kp5HiUDqtNMiX8+kGt5QfwLaZ+ROY9jj+Xnrg@mail.gmail.com","threadId":"49469","inReplyTo":"20181003150522.GQ23446@localhost","subject":"Re: [PATCH] coccicheck: process every source file at once","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2018-10-03T15:52:43Z","receivedAt":"2018-10-03T15:52:57Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Wed, Oct 3, 2018 at 8:05 AM SZEDER Gábor <szeder.dev@gmail.com> wrote:\n> In fact, it works finer than ever, because running 1.0.0 with this\n> patch on Travis CI notices a possible memmove() -> MOVE_ARRAY()\n> conversion:\n>\n>   https://travis-ci.org/szeder/git/jobs/436542684#L576\n>\n> Surprisingly, running 1.0.0 without this patch, or running 1.0.4\n> locally either with or without this patch doesn't notice that\n> memmove() call.  Presumably that's why Jonathan could kind-of \"revert\"\n> my conversion from f919ffebed (Use MOVE_ARRAY, 2018-01-22) in his\n> 6a1a79fd14 (object: move grafts to object parser, 2018-05-15) without\n> anyone noticing.\n>\n\nThat seems very odd...\n\nThanks,\nJake\n"},{"id":"359559","messageId":"CAGZ79kZKb_t5ALF6KPtKt05gt1jv5gZL_rrdmeO5vNAb7ed8bQ@mail.gmail.com","threadId":"49469","inReplyTo":"CA+P7+xpB5Pqj7Kp5HiUDqtNMiX8+kGt5QfwLaZ+ROY9jj+Xnrg@mail.gmail.com","subject":"Re: [PATCH] coccicheck: process every source file at once","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-03T17:54:54Z","receivedAt":"2018-10-03T17:55:08Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Oct 3, 2018 at 8:52 AM Jacob Keller <jacob.keller@gmail.com> wrote:\n>\n> On Wed, Oct 3, 2018 at 8:05 AM SZEDER Gábor <szeder.dev@gmail.com> wrote:\n> > In fact, it works finer than ever, because running 1.0.0 with this\n> > patch on Travis CI notices a possible memmove() -> MOVE_ARRAY()\n> > conversion:\n> >\n> >   https://travis-ci.org/szeder/git/jobs/436542684#L576\n> >\n> > Surprisingly, running 1.0.0 without this patch, or running 1.0.4\n> > locally either with or without this patch doesn't notice that\n> > memmove() call.  Presumably that's why Jonathan could kind-of \"revert\"\n> > my conversion from f919ffebed (Use MOVE_ARRAY, 2018-01-22) in his\n> > 6a1a79fd14 (object: move grafts to object parser, 2018-05-15) without\n> > anyone noticing.\n> >\n>\n> That seems very odd...\n\nThat looks like a bad rebase on my side as I was carrying that patch\nfor a while as the development of object store series took some time.\n\nAnd I agree we should re-introduce the MOVE_ARRAY there.\n"}]}