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