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

[Magnus Ihse Bursie]

Looks good to me too. Thanks for doing it this way. 

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