Looks good to me too. Thanks for doing it this way. /Magnus
> 19 apr. 2018 kl. 20:08 skrev Severin Gehwolf <[email protected]>: > > 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 >> >>
