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