Re: [PATCH] coccicheck: process every source file at once

From: Jeff King <peff@peff.net>
Date: Oct 2, 2018, 20:31 UTC
Message-ID: <20181002203124.GC2014@sigill.intra.peff.net>
In-Reply-To: <CA+P7+xrjpEe_3_mAwZ73O2JP2Pd518OFnSf+gfmbTJW7A3Q=Nw@mail.gmail.com>

On Tue, Oct 02, 2018 at 01:00:21PM -0700, Jacob Keller wrote:

Show 13 quoted lines

> > > 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).

Show 23 quoted lines

> > > 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