RFR: 8201788: Number of make jobs wrong for bootcycle-images target

Fri Apr 20 07:51:14 UTC 2018

Hi Magnus,

On Thu, 2018-04-19 at 20:58 +0200, Magnus Ihse Bursie wrote:
> Looks good to me too. Thanks for doing it this way. 

Thanks for the review, pushed (jdk-submit came back clean).

Cheers,
Severin

> /Magnus
> 
> > 19 apr. 2018 kl. 20:08 skrev Severin Gehwolf <sgehwolf at redhat.com>:
> > 
> > Hi Erik,
> > 
> > > On Thu, 2018-04-19 at 09:03 -0700, Erik Joelsson wrote:
> > > Hello,
> > > 
> > > > On 2018-04-19 08:58, Severin Gehwolf wrote:
> > > > Hi Erik,
> > > > 
> > > > Thanks for the review!
> > > > 
> > > > > On Thu, 2018-04-19 at 08:25 -0700, Erik Joelsson wrote:
> > > > > Hello Severin,
> > > > > 
> > > > > The suggested patch is not a good idea because by setting -j
> > > > > on the make
> > > > > command line in a sub make disables the job server. The job
> > > > > server is
> > > > > what makes it possible to do recursive make with a fixed
> > > > > global number
> > > > > of "jobs". If you do as you suggest, you essentially double
> > > > > the total
> > > > > number of available "jobs". The original make retains its
> > > > > number and the
> > > > > submake get a full other set of the same number of "jobs".
> > > > 
> > > > I'm confused. Isn't this what the status quo is? With the
> > > > difference
> > > > that it's currently setting JOBS="", thus allowing sub-make to
> > > > add any
> > > > number of jobs. It'll result in sub-make calling "make -j"
> > > > where '-j'
> > > > won't get an argument. If that's the case it's disabling the
> > > > job server
> > > > currently too, no? Then again, why would we see build failures?
> > > > I must
> > > > be missing something.
> > > 
> > > Ah, correct, the current code is also disabling the job server,
> > > that is 
> > > the core of the issue. :) I'm sorry I wasn't clear on that, it
> > > was just 
> > > so obvious in my world. Any -j flag in a sub make disables the
> > > job 
> > > server connection between the calling make an the sub make.
> > > Setting it 
> > > to -j without argument is going to wreck a lot more havoc than
> > > setting 
> > > it to something like close to "number-of-cpus", which your first 
> > > suggestion does. The former more or less creates a fork bomb,
> > > while the 
> > > latter only over allocates by a factor 2 at the worst.
> > 
> > OK. That does make it sound like that "disabling the job server"
> > and
> > creating more jobs are independent problems. I somehow thought in
> > my
> > naive world that disabling the job server puts an end to the fork-
> > bomb
> > ;-)
> > 
> > Thanks for the clarification.
> > 
> > > > > My suggestion was to explicitly turn off the setting of JOBS
> > > > > based on a
> > > > > special variable flag, just for bootcycle builds. Magnus
> > > > > didn't like
> > > > > that because introducing a lot of special flags everywhere
> > > > > will
> > > > > eventually lead to very convoluted code. He instead suggested
> > > > > that the
> > > > > bootcycle call should continue to set JOBS to empty, then the
> > > > > code in
> > > > > Init.gmk which sets the -j flag should be changed to:
> > > > > 
> > > > > $(if $(JOBS), -j=$(JOBS))
> > > > > 
> > > > > So that we only set -j if JOBS have a value. My only
> > > > > objection to that
> > > > > was that we then no longer support the case of letting make
> > > > > run with any
> > > > > number of jobs. I do agree that removing that option isn't a
> > > > > big deal.
> > > > > You can always work around it by setting JOBS to a very large
> > > > > number
> > > > > (like 1000, which is way more than any possible concurrency
> > > > > currently
> > > > > possible in the build anyway).
> > > > > 
> > > > > So to summarize, I think the correct solution to the bug is
> > > > > the snippet
> > > > > above.
> > > > 
> > > > Alright. How does this webrev look to you?
> > > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8201788/webrev
> > > > .01/
> > > 
> > > Yes, this looks good. Consider it reviewed.
> > 
> > Great, thanks for the review! I'm currently running this through
> > jdk-
> > submit. Hopefully I'll get some response this time :)
> > 
> > Cheers,
> > Severin
> > 
> > > /Erik
> > > > Thanks,
> > > > Severin
> > > > 
> > > > > /Erik
> > > > > 
> > > > > 
> > > > > > On 2018-04-19 07:46, Severin Gehwolf wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8201788
> > > > > > 
> > > > > > I'd like to get a fix in for an old discussion which
> > > > > > already happened a
> > > > > > while ago:
> > > > > > http://mail.openjdk.java.net/pipermail/build-dev/2017-April
> > > > > > /018929.html
> > > > > > 
> > > > > > The issue is that for bootcycle-images target a recursive
> > > > > > call to make
> > > > > > is being called with 'JOBS=""' which results in a call to
> > > > > > "make -j".
> > > > > > Thus, make is free to use as many jobs as it would like.
> > > > > > This may cause
> > > > > > for the occasional build failure. It has for us in the
> > > > > > past.
> > > > > > 
> > > > > > In this old thread above a patch like this was discouraged,
> > > > > > unless I
> > > > > > misunderstood something.
> > > > > > 
> > > > > > diff --git a/make/Main.gmk b/make/Main.gmk
> > > > > > --- a/make/Main.gmk
> > > > > > +++ b/make/Main.gmk
> > > > > > @@ -321,7 +321,7 @@
> > > > > >           ifneq ($(COMPILE_TYPE), cross)
> > > > > >            $(call LogWarn, Boot cycle build step 2:
> > > > > > Building a new JDK image using previously built image)
> > > > > >            +$(MAKE) $(MAKE_ARGS) -f $(TOPDIR)/make/Init.gmk
> > > > > > PARALLEL_TARGETS=$(BOOTCYCLE_TARGET) \
> > > > > > -             JOBS= SPEC=$(dir $(SPEC))bootcycle-spec.gmk
> > > > > > main
> > > > > > +             JOBS=$(JOBS) SPEC=$(dir $(SPEC))bootcycle-
> > > > > > spec.gmk main
> > > > > >           else
> > > > > >            $(call LogWarn, Boot cycle build disabled when
> > > > > > cross compiling)
> > > > > >           endif
> > > > > > 
> > > > > > It's my understanding that such a fix would pass down the
> > > > > > relevant JOBS
> > > > > > setting to sub-make and, thus, producing the desired 'make
> > > > > > -j <JOBS>'
> > > > > > call? What am I missing? If somebody wants to shoot
> > > > > > themselves in the
> > > > > > foot by doing:
> > > > > > 
> > > > > > configure ...
> > > > > > make JOBS=
> > > > > > 
> > > > > > That should be fine as it would just result in "make -j"
> > > > > > calls without
> > > > > > arguments. The common case where the JOBS setting comes
> > > > > > from configure
> > > > > > would be fixed, though. bootcycle-images target would
> > > > > > result in "make
> > > > > > -j <num-cores>".
> > > > > > 
> > > > > > Thoughts? Suggestions?
> > > > > > 
> > > > > > Thanks,
> > > > > > Severin
> > > 
> > > 
> 
> 