| Message ID | 20181002191642.21504-1-jacob.e.keller@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | coccicheck: process every source file at once | expand |
On Tue, Oct 02, 2018 at 12:16:42PM -0700, Jacob Keller wrote: > make coccicheck is used in order to apply coccinelle semantic patches, > and see if any of the transformations found within contrib/coccinelle/ > can be applied to the current code base. > > Pass every file to a single invocation of spatch, instead of running > spatch once per source file. > > This reduces the time required to run make coccicheck by a significant > amount of time: > > Prior timing of make coccicheck > real 6m14.090s > user 25m2.606s > sys 1m22.919s > > New timing of make coccicheck > real 1m36.580s > user 7m55.933s > sys 0m18.219s Yay! This is a nice result. It's also one of the things that Julia suggested in an earlier thread. One thing I wasn't quite sure about after digging into the various versions (1.0.4 on Debian stable/unstable, 1.0.6 in experimental, and pre-1.0.7 at the bleeding edge) was whether the old versions would be similarly helped (or work at all). I just replicated your results with 1.0.4.deb-3+b2 from Debian stable. It's possible there are older versions floating around, but for something developer-only like this, I think "in Debian stable" is a reasonable enough cutoff. > This is nearly a 4x decrease in the time required to run make > coccicheck. This is due to the overhead of restarting spatch for every > file. By processing all files at once, we can amortize this startup cost > across the total number of files, rather than paying it once per file. One minor side effect is that we lose the opportunity to parallelize quite as much. However, I think the reduction in total CPU makes it well worth that (and we still have 8 cocci files and growing, which should keep at least 8 cores busy). I think recent versions of Coccinelle will actually parallelize internally, too, but my 1.0.4 doesn't seem to. That's probably what I was thinking of earlier (but this is still a win even without that). It looks like this also fixes a problem I ran into when doing the oideq work, which is that the patch for a header file would get shown multiple times (once for each file that includes it). So this is faster _and_ more correct. Double-yay. > diff --git a/Makefile b/Makefile > index df1df9db78da..b9947f3f51ec 100644 > --- a/Makefile > +++ b/Makefile > @@ -2715,10 +2715,8 @@ endif > %.cocci.patch: %.cocci $(COCCI_SOURCES) > @echo ' ' SPATCH $<; \ > ret=0; \ > - for f in $(COCCI_SOURCES); do \ > - $(SPATCH) --sp-file $< $$f $(SPATCH_FLAGS) || \ > - { ret=$$?; break; }; \ > - done >$@+ 2>$@.log; \ > + ( $(SPATCH) --sp-file $< $(COCCI_SOURCES) $(SPATCH_FLAGS) || \ > + { ret=$$?; }; ) >$@+ 2>$@.log; \ This looks pretty straight-forward. I wondered if we could get rid of the "ret" handling, since we don't need to pass the error back out of the loop. But it's also used for the "show the log only on error" logic below: > if test $$ret != 0; \ > then \ > cat $@.log; \ The subshell could be a {}, though, I think, but it's not that big a deal either way. -Peff
On Tue, Oct 2, 2018 at 12:55 PM Jeff King <peff@peff.net> wrote: > > On Tue, Oct 02, 2018 at 12:16:42PM -0700, Jacob Keller wrote: > > > make coccicheck is used in order to apply coccinelle semantic patches, > > and see if any of the transformations found within contrib/coccinelle/ > > can be applied to the current code base. > > > > Pass every file to a single invocation of spatch, instead of running > > spatch once per source file. > > > > This reduces the time required to run make coccicheck by a significant > > amount of time: > > > > Prior timing of make coccicheck > > real 6m14.090s > > user 25m2.606s > > sys 1m22.919s > > > > New timing of make coccicheck > > real 1m36.580s > > user 7m55.933s > > sys 0m18.219s > > Yay! This is a nice result. > > It's also one of the things that Julia suggested in an earlier thread. > One thing I wasn't quite sure about after digging into the various > versions (1.0.4 on Debian stable/unstable, 1.0.6 in experimental, and > pre-1.0.7 at the bleeding edge) was whether the old versions would be > similarly helped (or work at all). > > I just replicated your results with 1.0.4.deb-3+b2 from Debian stable. > It's possible there are older versions floating around, but for > something developer-only like this, I think "in Debian stable" is a > reasonable enough cutoff. > Good. I hadn't checked back too far, but I know support for multiple files has existed since quite a while. > > This is nearly a 4x decrease in the time required to run make > > coccicheck. This is due to the overhead of restarting spatch for every > > file. By processing all files at once, we can amortize this startup cost > > across the total number of files, rather than paying it once per file. > > One minor side effect is that we lose the opportunity to parallelize > quite as much. However, I think the reduction in total CPU makes it well > worth that (and we still have 8 cocci files and growing, which should > keep at least 8 cores busy). > I don't think we do any less than previously, because we are doing each file in a for-loop, so those would all be serial anyways. > I think recent versions of Coccinelle will actually parallelize > internally, too, but my 1.0.4 doesn't seem to. That's probably what I > was thinking of earlier (but this is still a win even without that). > > It looks like this also fixes a problem I ran into when doing the oideq > work, which is that the patch for a header file would get shown multiple > times (once for each file that includes it). So this is faster _and_ > more correct. Double-yay. > Woot :D > > diff --git a/Makefile b/Makefile > > index df1df9db78da..b9947f3f51ec 100644 > > --- a/Makefile > > +++ b/Makefile > > @@ -2715,10 +2715,8 @@ endif > > %.cocci.patch: %.cocci $(COCCI_SOURCES) > > @echo ' ' SPATCH $<; \ > > ret=0; \ > > - for f in $(COCCI_SOURCES); do \ > > - $(SPATCH) --sp-file $< $$f $(SPATCH_FLAGS) || \ > > - { ret=$$?; break; }; \ > > - done >$@+ 2>$@.log; \ > > + ( $(SPATCH) --sp-file $< $(COCCI_SOURCES) $(SPATCH_FLAGS) || \ > > + { ret=$$?; }; ) >$@+ 2>$@.log; \ > > This looks pretty straight-forward. I wondered if we could get rid of > the "ret" handling, since we don't need to pass the error back out of > the loop. But it's also used for the "show the log only on error" logic > below: > We could probably get rid of it by doing the spatch without an ||, and then just save $?. > > if test $$ret != 0; \ > > then \ > > cat $@.log; \ > > The subshell could be a {}, though, I think, but it's not that big a > deal either way. > > -Peff
On Tue, Oct 02, 2018 at 01:00:21PM -0700, Jacob Keller wrote: > > > This is nearly a 4x decrease in the time required to run make > > > coccicheck. This is due to the overhead of restarting spatch for every > > > file. By processing all files at once, we can amortize this startup cost > > > across the total number of files, rather than paying it once per file. > > > > One minor side effect is that we lose the opportunity to parallelize > > quite as much. However, I think the reduction in total CPU makes it well > > worth that (and we still have 8 cocci files and growing, which should > > keep at least 8 cores busy). > > > > I don't think we do any less than previously, because we are doing > each file in a for-loop, so those would all be serial anyways. Yeah, sorry to be unclear. This is a strict improvement on what was there before. We had floated some patches to do the full NxM parallelization, but I think this path is better because of the reduction in actual CPU used (and if more recent versions of spatch can parallelize internally, we'll eventually get the best of both worlds). > > > diff --git a/Makefile b/Makefile > > > index df1df9db78da..b9947f3f51ec 100644 > > > --- a/Makefile > > > +++ b/Makefile > > > @@ -2715,10 +2715,8 @@ endif > > > %.cocci.patch: %.cocci $(COCCI_SOURCES) > > > @echo ' ' SPATCH $<; \ > > > ret=0; \ > > > - for f in $(COCCI_SOURCES); do \ > > > - $(SPATCH) --sp-file $< $$f $(SPATCH_FLAGS) || \ > > > - { ret=$$?; break; }; \ > > > - done >$@+ 2>$@.log; \ > > > + ( $(SPATCH) --sp-file $< $(COCCI_SOURCES) $(SPATCH_FLAGS) || \ > > > + { ret=$$?; }; ) >$@+ 2>$@.log; \ > > > > This looks pretty straight-forward. I wondered if we could get rid of > > the "ret" handling, since we don't need to pass the error back out of > > the loop. But it's also used for the "show the log only on error" logic > > below: > > > > We could probably get rid of it by doing the spatch without an ||, and > then just save $?. Actually, I guess we do not need to save $? at all, since we have only a single process to care about. So even simpler: spatch ... 2>$@+ 2>$@.log || { cat $@.log exit 1 } # if we get here, we were successful mv $@+ $@ ;# etc would work. That's missing all the Makefile=required backslashes and semicolons, of course. ;) -Peff
On Tue, Oct 2, 2018 at 1:31 PM Jeff King <peff@peff.net> wrote: > Actually, I guess we do not need to save $? at all, since we have only a > single process to care about. So even simpler: > > spatch ... 2>$@+ 2>$@.log || > { > cat $@.log > exit 1 > } > # if we get here, we were successful > mv $@+ $@ ;# etc > > would work. That's missing all the Makefile=required backslashes and > semicolons, of course. ;) > I opted to drop to just save the return, immediately after calling. It's a bit less code change, and I think the result is as clear as the above would be. This way we do drop the subshell, not that it matters much in the end... Thanks, Jake > -Peff
On Tue, Oct 02, 2018 at 01:58:10PM -0700, Jacob Keller wrote: > On Tue, Oct 2, 2018 at 1:31 PM Jeff King <peff@peff.net> wrote: > > Actually, I guess we do not need to save $? at all, since we have only a > > single process to care about. So even simpler: > > > > spatch ... 2>$@+ 2>$@.log || > > { > > cat $@.log > > exit 1 > > } > > # if we get here, we were successful > > mv $@+ $@ ;# etc > > > > would work. That's missing all the Makefile=required backslashes and > > semicolons, of course. ;) > > > > I opted to drop to just save the return, immediately after calling. > It's a bit less code change, and I think the result is as clear as the > above would be. This way we do drop the subshell, not that it matters > much in the end... Yeah. To be clear, I'm fine with any of the versions discussed in this thread. Thanks for working on this! -Peff
On Tue, Oct 02, 2018 at 03:55:19PM -0400, Jeff King wrote: > On Tue, Oct 02, 2018 at 12:16:42PM -0700, Jacob Keller wrote: > > > make coccicheck is used in order to apply coccinelle semantic patches, > > and see if any of the transformations found within contrib/coccinelle/ > > can be applied to the current code base. > > > > Pass every file to a single invocation of spatch, instead of running > > spatch once per source file. > > > > This reduces the time required to run make coccicheck by a significant > > amount of time: > > > > Prior timing of make coccicheck > > real 6m14.090s > > user 25m2.606s > > sys 1m22.919s > > > > New timing of make coccicheck > > real 1m36.580s > > user 7m55.933s > > sys 0m18.219s > > Yay! This is a nice result. > > It's also one of the things that Julia suggested in an earlier thread. > One thing I wasn't quite sure about after digging into the various > versions (1.0.4 on Debian stable/unstable, 1.0.6 in experimental, and > pre-1.0.7 at the bleeding edge) was whether the old versions would be > similarly helped (or work at all). > > I just replicated your results with 1.0.4.deb-3+b2 from Debian stable. > It's possible there are older versions floating around, but for > something developer-only like this, I think "in Debian stable" is a > reasonable enough cutoff. Linux build jobs on Travis CI run Ubuntu Trusty 14.04 LTS, and therefore our static analysis build job still runs 1.0.0~rc19.deb-3. This patch appears to work fine with that version, too, though note that I also changed the build job to don't run two parallel jobs; for the reason see below. > > This is nearly a 4x decrease in the time required to run make > > coccicheck. This is due to the overhead of restarting spatch for every > > file. By processing all files at once, we can amortize this startup cost > > across the total number of files, rather than paying it once per file. > > One minor side effect is that we lose the opportunity to parallelize > quite as much. However, I think the reduction in total CPU makes it well > worth that (and we still have 8 cocci files and growing, which should > keep at least 8 cores busy). One major side effect, however, is the drastically increased memory usage resulting from processing all files at once. With your patch on top of current master: $ for cocci in contrib/coccinelle/*.cocci ; do command time -f 'max RSS: %MkB' make ${cocci}.patch ; done SPATCH contrib/coccinelle/array.cocci max RSS: 1537068kB SPATCH contrib/coccinelle/commit.cocci max RSS: 1510920kB SPATCH contrib/coccinelle/free.cocci max RSS: 1393712kB SPATCH contrib/coccinelle/object_id.cocci SPATCH result: contrib/coccinelle/object_id.cocci.patch max RSS: 1831700kB SPATCH contrib/coccinelle/qsort.cocci max RSS: 1384960kB SPATCH contrib/coccinelle/strbuf.cocci max RSS: 1395800kB SPATCH contrib/coccinelle/swap.cocci max RSS: 1393620kB SPATCH contrib/coccinelle/xstrdup_or_null.cocci max RSS: 1371716kB Without your patch the max RSS lingers around 87048kB - 101980kB, meaning ~15x - 18x increase This might cause quite the surprise to developers who are used to run 'make -jN coccicheck'; my tiny laptop came to a grinding halt with -j4. I think this side effect should be mentioned in the commit message. Furthermore, the above mentioned static analysis build job on Travis CI runs two parallel jobs. Perhaps we should be considerate and reduce that to a single job, even if that means that he build job will run longer. > I think recent versions of Coccinelle will actually parallelize > internally, too, but my 1.0.4 doesn't seem to. That's probably what I > was thinking of earlier (but this is still a win even without that). I think Coccinelle parallelizes only when invoked with -j <N>, but that option is not documented in 1.0.4. I wrote "documented" instead of "present", because e.g.: $ spatch --sp-file contrib/coccinelle/swap.cocci -j 2 a*.c doesn't abort with "unknown option" and usage, but runs for a bit and then outputs: init_defs_builtins: /usr/lib/coccinelle/standard.h HANDLING: abspath.c advice.c alias.c alloc.c apply.c archive.c archive-tar.c archive-zip.c argv-array.c attr.c Fatal error: exception Sys_error("swap: No such file or directory") Without '-j 2' the command runs just fine. I don't know what to make of it. > It looks like this also fixes a problem I ran into when doing the oideq > work, which is that the patch for a header file would get shown multiple > times (once for each file that includes it). So this is faster _and_ > more correct. Double-yay. In that case I used to apply the change to the header first, and then apply the semantic patch one more time.
On Wed, Oct 03, 2018 at 12:16:58PM +0200, SZEDER Gábor wrote: > On Tue, Oct 02, 2018 at 03:55:19PM -0400, Jeff King wrote: > > On Tue, Oct 02, 2018 at 12:16:42PM -0700, Jacob Keller wrote: > > > > > make coccicheck is used in order to apply coccinelle semantic patches, > > > and see if any of the transformations found within contrib/coccinelle/ > > > can be applied to the current code base. > > > > > > Pass every file to a single invocation of spatch, instead of running > > > spatch once per source file. > > > > > > This reduces the time required to run make coccicheck by a significant > > > amount of time: > > > > > > Prior timing of make coccicheck > > > real 6m14.090s > > > user 25m2.606s > > > sys 1m22.919s > > > > > > New timing of make coccicheck > > > real 1m36.580s > > > user 7m55.933s > > > sys 0m18.219s > > > > Yay! This is a nice result. > > > > It's also one of the things that Julia suggested in an earlier thread. > > One thing I wasn't quite sure about after digging into the various > > versions (1.0.4 on Debian stable/unstable, 1.0.6 in experimental, and > > pre-1.0.7 at the bleeding edge) was whether the old versions would be > > similarly helped (or work at all). > > > > I just replicated your results with 1.0.4.deb-3+b2 from Debian stable. > > It's possible there are older versions floating around, but for > > something developer-only like this, I think "in Debian stable" is a > > reasonable enough cutoff. > > Linux build jobs on Travis CI run Ubuntu Trusty 14.04 LTS, and > therefore our static analysis build job still runs 1.0.0~rc19.deb-3. > > This patch appears to work fine with that version, too, In fact, it works finer than ever, because running 1.0.0 with this patch on Travis CI notices a possible memmove() -> MOVE_ARRAY() conversion: https://travis-ci.org/szeder/git/jobs/436542684#L576 Surprisingly, running 1.0.0 without this patch, or running 1.0.4 locally either with or without this patch doesn't notice that memmove() call. Presumably that's why Jonathan could kind-of "revert" my conversion from f919ffebed (Use MOVE_ARRAY, 2018-01-22) in his 6a1a79fd14 (object: move grafts to object parser, 2018-05-15) without anyone noticing.
On Wed, Oct 3, 2018 at 3:17 AM SZEDER Gábor <szeder.dev@gmail.com> wrote: > > On Tue, Oct 02, 2018 at 03:55:19PM -0400, Jeff King wrote: > > On Tue, Oct 02, 2018 at 12:16:42PM -0700, Jacob Keller wrote: > > > > > make coccicheck is used in order to apply coccinelle semantic patches, > > > and see if any of the transformations found within contrib/coccinelle/ > > > can be applied to the current code base. > > > > > > Pass every file to a single invocation of spatch, instead of running > > > spatch once per source file. > > > > > > This reduces the time required to run make coccicheck by a significant > > > amount of time: > > > > > > Prior timing of make coccicheck > > > real 6m14.090s > > > user 25m2.606s > > > sys 1m22.919s > > > > > > New timing of make coccicheck > > > real 1m36.580s > > > user 7m55.933s > > > sys 0m18.219s > > > > Yay! This is a nice result. > > > > It's also one of the things that Julia suggested in an earlier thread. > > One thing I wasn't quite sure about after digging into the various > > versions (1.0.4 on Debian stable/unstable, 1.0.6 in experimental, and > > pre-1.0.7 at the bleeding edge) was whether the old versions would be > > similarly helped (or work at all). > > > > I just replicated your results with 1.0.4.deb-3+b2 from Debian stable. > > It's possible there are older versions floating around, but for > > something developer-only like this, I think "in Debian stable" is a > > reasonable enough cutoff. > > Linux build jobs on Travis CI run Ubuntu Trusty 14.04 LTS, and > therefore our static analysis build job still runs 1.0.0~rc19.deb-3. > > This patch appears to work fine with that version, too, though note > that I also changed the build job to don't run two parallel jobs; for > the reason see below. > > > > This is nearly a 4x decrease in the time required to run make > > > coccicheck. This is due to the overhead of restarting spatch for every > > > file. By processing all files at once, we can amortize this startup cost > > > across the total number of files, rather than paying it once per file. > > > > One minor side effect is that we lose the opportunity to parallelize > > quite as much. However, I think the reduction in total CPU makes it well > > worth that (and we still have 8 cocci files and growing, which should > > keep at least 8 cores busy). > > One major side effect, however, is the drastically increased memory > usage resulting from processing all files at once. With your patch on > top of current master: > > $ for cocci in contrib/coccinelle/*.cocci ; do command time -f 'max RSS: %MkB' make ${cocci}.patch ; done > SPATCH contrib/coccinelle/array.cocci > max RSS: 1537068kB > SPATCH contrib/coccinelle/commit.cocci > max RSS: 1510920kB > SPATCH contrib/coccinelle/free.cocci > max RSS: 1393712kB > SPATCH contrib/coccinelle/object_id.cocci > SPATCH result: contrib/coccinelle/object_id.cocci.patch > max RSS: 1831700kB > SPATCH contrib/coccinelle/qsort.cocci > max RSS: 1384960kB > SPATCH contrib/coccinelle/strbuf.cocci > max RSS: 1395800kB > SPATCH contrib/coccinelle/swap.cocci > max RSS: 1393620kB > SPATCH contrib/coccinelle/xstrdup_or_null.cocci > max RSS: 1371716kB > > Without your patch the max RSS lingers around 87048kB - 101980kB, > meaning ~15x - 18x increase > Quite a significant increase. > This might cause quite the surprise to developers who are used to run > 'make -jN coccicheck'; my tiny laptop came to a grinding halt with > -j4. > > I think this side effect should be mentioned in the commit message. > Yes I agree. I hadn't considered the memory problem. Thanks, Jake
On Wed, Oct 3, 2018 at 8:05 AM SZEDER Gábor <szeder.dev@gmail.com> wrote: > In fact, it works finer than ever, because running 1.0.0 with this > patch on Travis CI notices a possible memmove() -> MOVE_ARRAY() > conversion: > > https://travis-ci.org/szeder/git/jobs/436542684#L576 > > Surprisingly, running 1.0.0 without this patch, or running 1.0.4 > locally either with or without this patch doesn't notice that > memmove() call. Presumably that's why Jonathan could kind-of "revert" > my conversion from f919ffebed (Use MOVE_ARRAY, 2018-01-22) in his > 6a1a79fd14 (object: move grafts to object parser, 2018-05-15) without > anyone noticing. > That seems very odd... Thanks, Jake
On Wed, Oct 3, 2018 at 8:52 AM Jacob Keller <jacob.keller@gmail.com> wrote: > > On Wed, Oct 3, 2018 at 8:05 AM SZEDER Gábor <szeder.dev@gmail.com> wrote: > > In fact, it works finer than ever, because running 1.0.0 with this > > patch on Travis CI notices a possible memmove() -> MOVE_ARRAY() > > conversion: > > > > https://travis-ci.org/szeder/git/jobs/436542684#L576 > > > > Surprisingly, running 1.0.0 without this patch, or running 1.0.4 > > locally either with or without this patch doesn't notice that > > memmove() call. Presumably that's why Jonathan could kind-of "revert" > > my conversion from f919ffebed (Use MOVE_ARRAY, 2018-01-22) in his > > 6a1a79fd14 (object: move grafts to object parser, 2018-05-15) without > > anyone noticing. > > > > That seems very odd... That looks like a bad rebase on my side as I was carrying that patch for a while as the development of object store series took some time. And I agree we should re-introduce the MOVE_ARRAY there.
diff --git a/Makefile b/Makefile index df1df9db78da..b9947f3f51ec 100644 --- a/Makefile +++ b/Makefile @@ -2715,10 +2715,8 @@ endif %.cocci.patch: %.cocci $(COCCI_SOURCES) @echo ' ' SPATCH $<; \ ret=0; \ - for f in $(COCCI_SOURCES); do \ - $(SPATCH) --sp-file $< $$f $(SPATCH_FLAGS) || \ - { ret=$$?; break; }; \ - done >$@+ 2>$@.log; \ + ( $(SPATCH) --sp-file $< $(COCCI_SOURCES) $(SPATCH_FLAGS) || \ + { ret=$$?; }; ) >$@+ 2>$@.log; \ if test $$ret != 0; \ then \ cat $@.log; \