Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parallel cmake and make #4783

Closed
wants to merge 4 commits into from

Conversation

publicarray
Copy link
Member

@publicarray publicarray commented Aug 15, 2021

Motivation: Make compiling faster. I've only tested fish and umurmur so far. (I have an i7-10750H with 6 cores)

# fish before
real	2m29.187s
user	2m58.331s
sys	0m29.212s

# fish after
real	0m49.710s
user	4m31.117s
sys	0m35.185s

# fish ninja build (https://github.com/publicarray/spksrc/tree/cmake-ninja)
real	1m8.352s
user	6m17.500s
sys	0m47.527s

# fish https://github.com/SynoCommunity/spksrc/pull/4803
real    0m49.367s
user    4m33.264s
sys     0m35.001s

fish from 2m29s to 49 s

# python38 before 
real    10m44.831s
user    15m10.705s
sys 2m10.672s

# python38 after
real    6m6.519s
user    13m9.923s
sys 1m52.586s

@@ -33,7 +33,7 @@ compile_msg:
pre_compile_target: compile_msg

compile_target: $(PRE_COMPILE_TARGET)
@$(RUN) $(MAKE) $(COMPILE_MAKE_OPTIONS)
@$(RUN) $(MAKE) -j$(NCPUS) $(COMPILE_MAKE_OPTIONS)
Copy link
Member Author

@publicarray publicarray Aug 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm not sure if this one does anything

Copy link
Contributor

@hgy59 hgy59 Aug 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes it is obsolete, as COMPILE_MAKE_OPTIONS += -j$(NCPUS) is already defined above in this file.
But all custom COMPILE_MAKE_OPTIONS must be defined with += too, to keep the -j option.

Copy link
Member Author

@publicarray publicarray Aug 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks having it multiple times doesn't actually hurt AFAIK but makes things messy. Impressive improvements though when packages use the $(COMPILE_MAKE_OPTIONS) aka include the -j$(NCPUS)

@publicarray publicarray changed the title Parallel cmake Parallel cmake and make Aug 15, 2021
Comment on lines 28 to 32
.PHONY: busybox_compile
busybox_compile:
$(RUN) CROSS_COMPILE=$(TC_PATH)$(TC_PREFIX) $(MAKE) busybox
$(RUN) CROSS_COMPILE=$(TC_PATH)$(TC_PREFIX) $(MAKE) -j$(NCPUS) busybox

.PHONY: busybox_install
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

most of the custom compile-targets adjusted here could be removed by definition of COMPILE_MAKE_OPTIONS
as the default compile_target executes $(RUN) $(MAKE) $(COMPILE_MAKE_OPTIONS)

busybox_compile can be removed by:
COMPILE_MAKE_OPTIONS += CROSS_COMPILE=$(TC_PATH)$(TC_PREFIX) busybox

this work only, when it does not matter that CROSS_COMPILE definition must not be left of $(MAKE) on the command line.

As spksrc.common.mk already defines COMPILE_MAKE_OPTIONS += -j$(NCPUS) this must not be added here again.

@@ -22,7 +22,7 @@ include ../../mk/spksrc.native-cc.mk

.PHONY: elixir_compile
elixir_compile:
$(RUN) PATH=$$PATH:$(ERLANG_BIN_DIR) $(MAKE) $(MAKE_COMPILE_OPTIONS)
$(RUN) PATH=$$PATH:$(ERLANG_BIN_DIR) $(MAKE) -j$(NCPUS) $(MAKE_COMPILE_OPTIONS)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here the PATH definition must preceed the $(MAKE) call, but we can omit the -j$(NCPUS) , as it is already in MAKE_COMPILE_OPTIONS.

@publicarray publicarray requested a review from th0ma7 August 22, 2021 14:24
@th0ma7
Copy link
Contributor

th0ma7 commented Aug 22, 2021

Humm, rather interesting but there is a need to be really really careful with this... There are two things to be on the lookout:

  1. Is this being called as a parallel build in the first place?
  2. Will you overrun your total CPU core count?

Let's say you run make -jX all-supported (X being nproc). This will result in parallel-build up to npcroc times of arch-<arch>-<version>. Now if you add parallel builds within each cross/* you may end-up with a npcroc * npcroc resulting in overwhelming your system entirely.

Currently, as we call -@MAKEFLAGS= $(MAKE) ... to invoke parallel building of arch-<arch>-<version> so you won't know if a -j was passed over to make. So there would be a need to check that first, and only then when assuming not already building in parallel (or not exceeding the total amount of core), add parallel building of cross*.

@publicarray
Copy link
Member Author

publicarray commented Aug 23, 2021

Hm that is a valid concern. I still think this is an overall improvement and adds consistency. AFAIK lots of packages are already running like you describe (npcroc * npcroc). I guess my focus is to make one arch builds faster to increase development productivity, everything else is secondary IMHO. As for overwhelming your system. It depends entirely how many cores you let docker use.

I guess my ultimate aim is to make one arch builds faster to increase development productivity and let the CI build all other architectures.

@th0ma7
Copy link
Contributor

th0ma7 commented Aug 23, 2021

I took a totally different approach (ref: b62495f), based on my current PR while fixing cross/* direct arch builds (PR #4803).

Running quick tests on my 12x core AMD show a significant reduction of build time for both regular make and cmake. Actually, zlib was sooo fast that I didn't thought it had built in the first place:

cross/SVT-AV1

$ time PARALLEL_MAKE=nop make arch-apollolake-7.0
real	2m51.662s
user	2m38.332s
sys	0m13.549s

$ time make arch-apollolake-7.0
real	0m32.660s
user	4m4.555s
sys	0m18.168s

cross/ZLIB

$ time PARALLEL_MAKE=nop make arch-apollolake-7.0
real	0m3.024s
user	0m2.190s
sys	0m0.888s

$ time make arch-apollolake-7.0
real	0m0.876s
user	0m2.553s
sys	0m1.146s

@publicarray
Copy link
Member Author

publicarray commented Aug 25, 2021

spk/fish

Before (current master):

$ make -C spk/fish clean
$ time make -C spk/fish all-supported
real    22m45.112s
user    27m26.899s
sys     4m33.054s

After (this PR):

$ make -C spk/fish clean
$ time make -C spk/fish all-supported
real    7m33.658s
user    40m11.263s
sys     5m12.594s

$ make -C spk/fish clean
$ time make -j12 -C spk/fish all-supported
Out of memory (over 18GB)

With your PR: (I forgot to clean after 1 arch was build)

$ time make -j12 -C spk/fish all-supported
real    2m19.985s
user    19m54.434s
sys     2m30.063s

@publicarray
Copy link
Member Author

Superseded by #4803

@publicarray publicarray deleted the parallel-cmake branch August 31, 2021 01:17
@publicarray publicarray restored the parallel-cmake branch August 31, 2021 01:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants