Enable multiple jobs for make when build from source #752

PeterDaveHello · 2015-05-23T05:36:24Z

Should build parallelly to make the whole process faster.

ljharb · 2015-05-23T05:48:33Z

Can you explain a bit more about what's happening in this PR?

ljharb · 2015-05-23T05:49:07Z

nvm.sh

@@ -1043,6 +1043,9 @@ nvm_install_node_source() {
  if [ "_$NVM_OS" = "_freebsd" ]; then
    make='gmake'
    MAKE_CXX="CXX=c++"
+    MAKE_JOBS="`sysctl hw.ncpu | awk '{print $2}'`"


please use $() instead of backticks; same below

PeterDaveHello · 2015-05-23T06:04:09Z

man make

   -j [jobs], --jobs[=jobs]
        Specifies the number of jobs (commands) to run simultaneously.  If there is more than one -j option, the last one is effective.  If the -j option is given without  an  argu‐
        ment, make will not limit the number of jobs that can run simultaneously.

If you have multi-core/processor platform, should let them work in parallel to speed up the build, there is no reason to use only one thread to build on multi-core/processor computer.

ljharb · 2015-05-23T06:05:57Z

Thank you, but i mean, what is nproc and sysctl, and what does MAKE_JOBS actually represent?

What happens now that this is set? Is there any reason a user might not want this setting turned on?

PeterDaveHello · 2015-05-23T06:06:37Z

@ljharb It's use to determinate how many thread(s) should make use.

ljharb · 2015-05-23T06:07:19Z

So is nproc the number of processors the machine has? Wouldn't it be better not to use 100% of available CPUs, but instead N - 1?

PeterDaveHello · 2015-05-23T06:09:52Z

No, we usually set to N + 1, N is fine enough, you can see many and many examples on the Internet.

Examples:
http://vim.wikia.com/wiki/Auto-detect_number_of_cores_for_parallel_build
https://unix.stackexchange.com/questions/51815/why-people-recommend-the-j3-option-for-make-when-having-a-dual-core-cpu
https://stackoverflow.com/questions/2499070/gnu-make-should-j-equal-number-the-number-of-cpu-cores-in-a-system

ljharb · 2015-05-23T06:26:32Z

On my Mac, I get -bash: nproc: command not found.

Any commands we use must be POSIX-compliant.

PeterDaveHello · 2015-05-23T06:34:14Z

Should work on mac now.

ljharb · 2015-05-23T07:25:03Z

nvm.sh

+  elif [ "_$NVM_OS" = "_sunos" ]; then
+    MAKE_JOBS="$(psrinfo | wc -l)"
+  else
+    MAKE_JOBS="1"


Is there a chance that any of the other commands could fail, making MAKE_JOBS be the empty string, and not hitting this else block?

It will be very special case I think, but if you want, I can modify it to:

if [ "_$NVM_OS" = "_linux" ]; then MAKE_JOBS="$(nproc)" elif [ "_$NVM_OS" = "_freebsd" ] || [ "_$NVM_OS" = "_darwin" ]; then MAKE_JOBS="$(sysctl -n hw.ncpu)" elif [ "_$NVM_OS" = "_sunos" ]; then MAKE_JOBS="$(psrinfo | wc -l)" fi if [ -z "$MAKE_JOBS" ]; then MAKE_JOBS="1" fi

I agree, but I think that'd be more reliable. Thanks for being diligent!

I would like to add additional info in that part, hope you don't mind.

ljharb · 2015-05-24T01:55:14Z

Looks like raspbian, ie a raspberry pi, might not have any of these three commands - also centOS 5.5 and Ubuntu 9.10 might not. However, grep -c 'core id' /proc/cpuinfo should work on any system where [ -f /proc/cpuinfo ]

PeterDaveHello · 2015-05-24T07:24:47Z

I think we can assume linux will have /proc/cpuinfo

Efreak · 2015-06-07T03:08:49Z

grep -c 'core id' /proc/cpuinfo should work on any system where [ -f /proc/cpuinfo ]

This is incorrect. I'm running a quad-core odroid-c1 with trusty on it, and /proc/cpuinfo doesn't contain 'core id'.

There's probably a few dozen ways of obtaining this information, however the most portable solution of all is likely to be getconf _NPROCESSORS_ONLN; getconf is part of the POSIX specification, and _NPROCESSORS_ONLN isn't mandatory, but is supported by both OS X and linux. Even cygwin supports it (though nodejs still doesn't support cygwin 😦)

The best way, however, might also be to just allow passing of custom parameters to the build system.

ljharb · 2015-06-07T03:17:29Z

Sounds like getconf _NPROCESSORS_ONLN plus an installation parameter to override the processor number may be the simplest/best approach here?

PeterDaveHello · 2015-06-08T16:22:45Z

hmmmmm ... anyway, we can fallback to 1 if not detect, and there is also warning msg, why not just enable this feature first and improve it in future? Current code should work on most of the systems.

ljharb · 2015-06-08T20:11:53Z

The concern would be that building on more processors without informing the user might be taking more processor resources than the user actually wants it to take.

PeterDaveHello · 2015-09-27T13:34:53Z

What about adding an option for non-multithreading compiling?

ljharb · 2015-09-27T13:42:22Z

@PeterDaveHello That could be fine. I'm concerned about the default, though. Perhaps it's OK though if the default is N - 1, and it can be overridden to any number the user wants?

PeterDaveHello · 2015-09-27T13:44:14Z

Agree!

PeterDaveHello · 2015-12-06T06:35:01Z

@ljharb I just updated this PR, now it support -j paramater followed -s to specified the CPU threads we can use, can you give me advice about the document?

ljharb · 2015-12-06T23:01:21Z

nvm.sh

@@ -1231,6 +1231,25 @@ nvm_install_node_source() {
    make='gmake'
    MAKE_CXX="CXX=c++"
  fi
+
+  if [ -z "$NVM_CPU_THREADS" ] || [[ ! $NVM_CPU_THREADS =~ ^[0-9]+$ ]] ; then


this syntax is bash-specific - nvm must work on all POSIX systems, including ksh, dash, sh, bash, and zsh.

oh okay, I'll modify it.

PeterDaveHello · 2015-12-21T09:26:59Z

@ljharb I'm not sure why this test failed: https://travis-ci.org/creationix/nvm/jobs/98080035, do you mind to point me out? Thanks a lot!

PeterDaveHello · 2015-12-21T13:08:58Z

Failed on the new installation test of sh/dash but didn't see any useful info in the build log...

ljharb · 2015-12-22T17:39:11Z

You can definitely ignore the nvm_reset install_script failure for now. The others seem legitimate.

PeterDaveHello · 2015-12-23T06:15:44Z

Yeah, and I can't pass the test I wrote now 😈 trying to fix it

PeterDaveHello · 2015-12-23T07:52:38Z

I just dropped a robustness test of giving non-expected value for -j, not so required.

PeterDaveHello · 2015-12-23T08:51:19Z

@ljharb is it good to merge now?

ljharb · 2015-12-25T17:47:39Z

nvm.sh

+      echo "Please report an issue on GitHub to help us make it better and run it faster on your computer!" 1>&2
+      NVM_MAKE_JOBS="1"
+    else
+      NVM_MAKE_JOBS=$(($NVM_CPU_THREADS - 1))


can we output something to stdout here indicating what the number of threads has been automatically set to?

ljharb · 2015-12-25T17:48:50Z

When I tried nvm install -j foo -s 0.12.2 I got grep: invalid option -- j although nvm install -s -j foo 0.12.2 worked as expected.

PeterDaveHello · 2015-12-26T10:06:32Z

When I tried nvm install -j foo -s 0.12.2 I got grep: invalid option -- j although nvm install -s -j foo 0.12.2 worked as expected.

Yep I didn't handle out of order input yet, any suggestion on this?

PeterDaveHello · 2015-12-26T13:15:19Z

Updated for output first.

ljharb · 2015-12-26T16:07:09Z

For the argument ordering: I think you'll want to loop through the arguments, stopping when you get to the first one that's not -s or -j - and when you find one of those, set the appropriate vars (and for the case of -j, use shift to grab the next item, naturally). That should be easy to extend in the future if we add more nvm install arguments.

PeterDaveHello · 2015-12-27T07:06:40Z

Can we implement that as another feature? Maybe this PR is just for essential support for multiple threads.

ljharb · 2015-12-27T16:08:13Z

@PeterDaveHello sure, can you rebase once more? that install_script test won't fail anymore

PeterDaveHello · 2015-12-27T17:37:37Z

Okay, rebased. Do you know the problem that cause build failed before?

ljharb · 2015-12-27T17:38:44Z

@PeterDaveHello yes, travis-ci/travis-ci#5363 is the culprit

PeterDaveHello · 2015-12-27T17:39:19Z

👍

PeterDaveHello · 2015-12-27T18:03:11Z

Test passed 😄

[New] `nvm install`: Enable multiple jobs for make when build from source via `-j`

ljharb · 2015-12-27T21:08:17Z

nvm.sh

+          NVM_CPU_THREAD_VALID=$(nvm_is_natural_num $1)
+          if [ "$NVM_CPU_THREAD_VALID" = "true" ]; then
+            NVM_MAKE_JOBS=$1
+            echo "Set number of jobs to $MAKE_JOBS for 'make' utility"


this var name is incorrect (i'll fix in a followup)

ljharb · 2015-12-27T23:07:50Z

Followup: f279837

PeterDaveHello · 2015-12-28T05:52:36Z

👍

ljharb reviewed May 23, 2015
View reviewed changes

ljharb mentioned this pull request Sep 23, 2015

Multithread node compiling #843

Closed

ljharb added installing node Issues with installing node/io.js versions. feature requests I want a new feature in nvm! needs rebase Please rebase your branch onto latest master! This removes merge commits, & keeps the git log clean. labels Sep 27, 2015

ljharb reviewed Dec 6, 2015
View reviewed changes

PeterDaveHello changed the title ~~Enable multiple jobs for when build from source~~ Enable multiple jobs for make when build from source Dec 23, 2015

ljharb reviewed Dec 25, 2015
View reviewed changes

PeterDaveHello added 4 commits December 28, 2015 01:37

Enable multiple jobs for when build from source

a8e6fee

Support parameter -j to set jobs for make

6320719

fix CPU_THREADS/CPU_THREAD_VALID/MAKE_JOBS prefix

e78dc51

add test for install from source

cfccf03

ljharb added a commit that referenced this pull request Dec 27, 2015

Merge pull request #752 from PeterDaveHelloKitchen/parallel_build

d40eca8

[New] `nvm install`: Enable multiple jobs for make when build from source via `-j`

ljharb merged commit d40eca8 into nvm-sh:master Dec 27, 2015

ljharb reviewed Dec 27, 2015
View reviewed changes

PeterDaveHello deleted the parallel_build branch December 28, 2015 05:52

Uh oh!

Enable multiple jobs for make when build from source #752

Enable multiple jobs for make when build from source #752

Uh oh!

Conversation

PeterDaveHello commented May 23, 2015

Uh oh!

ljharb commented May 23, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PeterDaveHello commented May 23, 2015

Uh oh!

ljharb commented May 23, 2015

Uh oh!

PeterDaveHello commented May 23, 2015

Uh oh!

ljharb commented May 23, 2015

Uh oh!

PeterDaveHello commented May 23, 2015

Uh oh!

ljharb commented May 23, 2015

Uh oh!

PeterDaveHello commented May 23, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ljharb commented May 24, 2015

Uh oh!

PeterDaveHello commented May 24, 2015

Uh oh!

Efreak commented Jun 7, 2015

Uh oh!

ljharb commented Jun 7, 2015

Uh oh!

PeterDaveHello commented Jun 8, 2015

Uh oh!

ljharb commented Jun 8, 2015

Uh oh!

PeterDaveHello commented Sep 27, 2015

Uh oh!

ljharb commented Sep 27, 2015

Uh oh!

PeterDaveHello commented Sep 27, 2015

Uh oh!

PeterDaveHello commented Dec 6, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PeterDaveHello commented Dec 21, 2015

Uh oh!

PeterDaveHello commented Dec 21, 2015

Uh oh!

ljharb commented Dec 22, 2015

Uh oh!

PeterDaveHello commented Dec 23, 2015

Uh oh!

PeterDaveHello commented Dec 23, 2015

Uh oh!

PeterDaveHello commented Dec 23, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ljharb commented Dec 25, 2015

Uh oh!

PeterDaveHello commented Dec 26, 2015

Uh oh!

PeterDaveHello commented Dec 26, 2015

Uh oh!

ljharb commented Dec 26, 2015