-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Fix per-arch builds & add multi-arch & single arch parallel builds #4803
Conversation
@hgy59 This solves the issue (can't find from which #issue) where Was there anything else to look into as well? PS: slowly catching up... |
@hgy59 and @publicarray this is good for your review. |
Would you mind doing one benchmark with |
@publicarray I noticed one yesterday... I am currently figuring out how to best handle the
Preliminary code for this is in the work... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my testing if max
is used this works as intended and improves compile time. But I'm not too qualified to talk about the .PARALLEL
and .NOTPARALLEL
commands.
Are there any packages that don't want to have the -j
option because it breaks something? This PR adds it by default, just something to consider. But I'm all for it 👍
mk/spksrc.cross-env.mk
Outdated
#endif | ||
# Set parallel options in caller | ||
ifeq ($(PARALLEL_MAKE),max) | ||
MAKEFLAGS += -j$(shell nproc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens when PARALLEL_MAKE is a number? IIRC PARALLEL_MAKE was used to limit how many cores to use. I guess it doesn't actually matter, since we can just limit docker to the number of cores we want to give it. Any thoughts @hgy59? If the new behaviour stays, rather than max, true/false would make more sense then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$ time PARALLEL_MAKE=3 make -C spk/fish arch-x64-7.0
real 3m8.738s
user 2m44.456s
sys 0m24.859s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@publicarray some early code and excellent results in hope to address this :
spksrc@spksrc-debian10:~/all-supported/spksrc/spk/lua$ time PARALLEL_MAKE=nop make all-supported
real 12m1.839s
user 8m35.287s
sys 3m39.685s
spksrc@spksrc-debian10:~/all-supported/spksrc/spk/lua$ time make all-supported
real 4m10.488s
user 10m42.093s
sys 5m19.238s
spksrc@spksrc-debian10:~/all-supported/spksrc/spk/lua$ time make -j4 all-supported
real 2m23.854s
user 10m3.423s
sys 4m35.352s
spksrc@spksrc-debian10:~/all-supported/spksrc/spk/lua$ time PARALLEL_MAKE=max make -j6 all-supported
real 2m5.130s
user 10m7.812s
sys 4m37.762s
spksrc@spksrc-debian10:~/all-supported/spksrc/spk/lua$ time PARALLEL_MAKE=4 make -j4 all-supported
real 1m54.972s
user 10m40.255s
sys 5m6.501s
spksrc@spksrc-debian10:~/all-supported/spksrc/spk/lua$ time PARALLEL_MAKE=6 make -j6 all-supported
real 1m47.070s
user 11m42.589s
sys 5m29.545s
This commit also add the ability to create stats from parallel make being invoked
@publicarray can you give this a shot? My initial take looks promising. Basically it is capable of following the
By default it defaults to max with no
Add the following to you
You can then try it out using variations on the theme such as (with combinations of
Having There currently is one corner-case that I'll have to dig a little more into, it's when building under the |
Awesome work. Pretty please rename PMAKE back to PARALLEL_MAKE. It's also used in GitHub actions and I also think the longer name is more descriptive to what it does. |
@publicarray looks pretty good now. I think all issues you found so far are now solved. Item still remaining (for a later time) is to allow full parallel build within EDIT: I found where the issue is with the parallel build within |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy with this.
Before merging, I do want to give @hgy59 a little time to have a look if he wants to.
@publicarray I was finally able to circumvent the issue with building from Long story short, to use As such the And sure, such a change requires a few pair of eyes (@hgy59) to confirm all looks good. I really wanted to complete this before disconnecting for the comming week. If you find it relevant for a merge please proceed as you see fit in my absence, cheers! |
@publicarray while playing a bit with #4797 I noticed a bizarre failure that may be due to this PR. I'm unable to reproduce locally, perhaps can you? I'm thinking perhaps we should revert this PR and look at it a little further? I could then add a "false" change to |
Hm what failures? Would it help to remove the -j flag if this is a package specific issue? To remove the |
Do you mean
? |
@publicarray actually, #4849 solves it entirely. |
…ynoCommunity#4803) * spk.mk: Handle properly arch-<arch> vs arch-<arch>-X.Y use cases * spk.mk: Rework code and optimize for parallel builds * spk.mk: Allow full parallel builds * spk.mk: Optimize parallel build functionality This commit also add the ability to create stats from parallel make being invoked * spk.mk: rename PMAKE back to PARALLEL_MAKE * spk.mk: Revert NCPUS, force download to .NOTPARALLEL, fix cmake * cmake.mk: Use NCPUS instead of call to shell * cmake.mk: No need to adjust MAKEFLAGS for cmake * compile.mk: Move MKFLAGS to compile.mk and set if MAKELEVEL>=2 * cross-cc.mk: Allow either -jX or PARALLEL_MAKE from cross/*
Motivation: Since code was changed to
all-supported
in order to allow full parallel builds and align github action workflow, it wasn't possible to build package simply usingmake arch-apollolake
(required full version such asmake arch-apollolake-7.0
). The following code change does:$DEFAULT_TC
packages for same arch when no version passed-jX
whith multiple archs passed in argument such asmake -j12 arch-evansport arch-apollolake-7.0
. Can be disabled withPARALLEL_MAKE=nop
make arch-apollolake-7.0
)build-<arch>-<version>.log
for arch builds (e.g. similar to group buildsall-suported
) forspk/*
andcross/*
Linked issues: #4783 (in theory it supersedes its parallel build functionality)
Checklist
all-supported
completed successfullyTest restults on AMD Ryzen 5 1600xX - 6x cores / 12 threads
spk/ffmpeg
spk/fish
spk/python38
spk/rutorrent
cross/bzip2
cross/lua
cross/svt-av1
spk/umurmur
cross/x265
cross/zlib