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

doc: add note to parallelize make #9961

Closed
wants to merge 2 commits into from

Conversation

jmdarling
Copy link
Contributor

Checklist
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

doc

Description of change

Adds a note to the BUILDING doc to encourage parallelizing make. When I
first built node I didn't know this trick and thought that the build was
just stuck in an infinite loop after waiting for 10 minutes.

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Dec 1, 2016
@mscdex mscdex added the build Issues and PRs related to build files or the CI. label Dec 1, 2016
@mscdex
Copy link
Contributor

mscdex commented Dec 1, 2016

Perhaps it would be even better to mention what the number after '-j' means.

@imyller imyller added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Dec 1, 2016
@jmdarling
Copy link
Contributor Author

Very good point. I added a short description (as I don't understand the gory details myself) and also added a link to the official GNU make parallelism documentation for those interested in the details. Do you think that's excessive?

@@ -36,6 +36,8 @@ $ make

Note that the above requires that `python` resolve to Python 2.6 or 2.7 and not a newer version.

*note: Running make in parallel, ```$ make -j8``` where ```-j8``` specifies that 8 recipes should run concurrently, can significantly reduce build time. See the [GNU Make Documentation](https://www.gnu.org/software/make/manual/html_node/Parallel.html) for more information.*

Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing that most people won't know what a recipe is so maybe make that "compile job" or something like that?

Style nit: can you wrap lines at 80 columns? You also don't need to use triple backticks, single ones suffice. I wouldn't necessarily use italics either but that's more of a personal preference.

@Fishrock123
Copy link
Member

I think we should add -j4 directly to the example above (the ./configure then make).

This is based on what I saw at the code and learn; sure it would be great if people read more but typically they start running stuff in order (which is reasonable for people who are new to something).

So... it seems somewhat odd to tell people to build without it and then come back around and say to do it with it. It would be best for the general experience for it to be directly there, with notes if it does not work and for additional processor cores.

@Fishrock123 Fishrock123 self-assigned this Dec 2, 2016
@gibfahn
Copy link
Member

gibfahn commented Dec 3, 2016

I guess the question is whether using make -j4 (or even -j8) ever causes appreciable slowdown or other issues on the kind of machine that people who are new to make are using. I'd argue that it won't, and it will help the vast majority of people. If you're running on a machine that can't handle 4/8 threads, then you probably know what you're doing with make.

If most people have 2/4 core hyperthreaded laptops, I'd say just use make -j8 like we do in CONTRIBUTING.md.

@Fishrock123
Copy link
Member

See also #8286.

There is some discussion there but from my observations the average appears to be 4 threads (some have less or more).

-j8 is nice if you can do it but it may be a little hard on lower-end laptops?

@jmdarling
Copy link
Contributor Author

I have a coworker who just got a new dual core MacBook Pro (I believe it is hyper-threaded). I'll ask him to run make with -j8, -j4, -j2, and without parallelism today and get some metrics on how long each took.

I realize that it will be extremely unscientific, but it may give us a bit of insight.

Side note, it would be really cool if we could figure out a way to gather some metrics on what kind of machines people are using to develop node and node projects. Maybe eventually we can send out a hardware survey like the user survey that was just sent out?

@evanlucas
Copy link
Contributor

This actually came up during the code and learn for someone with a MacBook Air. We had to switch to -j4 since they couldn't even use their browser with -j8

@jmdarling
Copy link
Contributor Author

Really Really Unscientific Benchmark Results:

Values taken from running $ time make -j[n]. All builds were run immediately after deleting the ./out directory. Both computers were 100% usable during the build without any signs of slowdown and were being used for web browsing and simple text editing.

I ran out of time with my coworker's laptop to run it without concurrency but that's kind of irrelivent to the current conversation anyway.

Mid 2012 Macbook Pro - 2.3GHz i7 - Hyper-Threaded Quad Core

make -j8 2746.33s user 260.92s system 601% cpu 8:19.88 total

make -j4 1805.77s user 192.59s system 370% cpu 8:59.70 total

make -j2 1721.45s user 165.89s system 191% cpu 16:24.13 total

Late 2016 Macbook Pro - 2.0GHz i5 - Hyper-Threaded Dual Core

make -j8 1904.60s user 150.79s system 347% cpu 9:51.45 total

make -j4 1851.63s user 145.75s system 339% cpu 9:48:07 total

make -j2 1244.67s user 115.51s system 183% cpu 12:20.78 total

Running with -j4 makes quite a bit of difference on both machines, but stepping up to -j8 doesn't really affect the dual core and only gives a minimal boost to the quad core.

Seeing these extremely unscientific results, and taking into account that -j8 is known to cause issues on at least one configuration, I think I'd personally lean toward recommending -j4 as it gives the best "bang for the buck" and is less likely to cause issues on lower end machines.

@jmdarling
Copy link
Contributor Author

I went ahead and changed the docs to use -j4 for this PR as it seems like this is probably the best option. I'd be happy to change it back to -j8 if that's deemed to be the better option.

If we want to standardize on -j4 I'd be happy to go through the docs and update them to be consistent either in this PR or in a separate issue.

It also may be worth the effort for us to have our own doc on make that we can link to wherever the -j[n] flag is used in the documentation, though this may be a bit overkill.

@gibfahn
Copy link
Member

gibfahn commented Dec 6, 2016

If we want to standardize on -j4 I'd be happy to go through the docs and update them to be consistent either in this PR or in a separate issue.

+1, might as well do it in this PR

@Fishrock123
Copy link
Member

@bnoordhuis LGTY now? This looks land-able to me!

@Fishrock123
Copy link
Member

@jmdarling This isn't patching cleanly for me, do you think you'd be able to squash this into two commits (one for the note, one for the standardization of -j4)?

jmdarling pushed a commit to jmdarling/node that referenced this pull request Dec 8, 2016
Standardizes docs to use -j4 instead of -j8 as it appears to be the
most inclusive recommendation based on discussion in
nodejs#9961.
Jonathan Darling added 2 commits December 7, 2016 22:58
Adds a note to the BUILDING doc to encourage parallelizing make. When I
first built node I didn't know this trick and thought that the build was
just stuck in an infinite loop after waiting for 10 minutes.
Standardizes docs to use -j4 instead of -j8 as it appears to be the
most inclusive recommendation based on discussion in
nodejs#9961.
@jmdarling
Copy link
Contributor Author

Sorry for the slow turnaround on that. My git-fu isn't what it should be and that simple task turned into an arduous journey 😝.

@Fishrock123
Copy link
Member

Landed in d8c7534...0cd1f54, thanks for the contribution! :D

@Fishrock123 Fishrock123 closed this Dec 9, 2016
Fishrock123 pushed a commit that referenced this pull request Dec 9, 2016
Adds a note to the BUILDING doc to encourage parallelizing make. When I
first built node I didn't know this trick and thought that the build was
just stuck in an infinite loop after waiting for 10 minutes.

Refs: #8286
Refs: #9881

PR-URL: #9961
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Fishrock123 pushed a commit that referenced this pull request Dec 9, 2016
Standardizes docs to use -j4 instead of -j8 as it appears to be the
most inclusive recommendation based on discussion in
#9961.

PR-URL: #9961
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Fishrock123 pushed a commit that referenced this pull request Dec 13, 2016
Adds a note to the BUILDING doc to encourage parallelizing make. When I
first built node I didn't know this trick and thought that the build was
just stuck in an infinite loop after waiting for 10 minutes.

Refs: #8286
Refs: #9881

PR-URL: #9961
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Fishrock123 pushed a commit that referenced this pull request Dec 13, 2016
Standardizes docs to use -j4 instead of -j8 as it appears to be the
most inclusive recommendation based on discussion in
#9961.

PR-URL: #9961
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
@italoacasas italoacasas mentioned this pull request Dec 15, 2016
MylesBorins pushed a commit that referenced this pull request Jan 16, 2017
Adds a note to the BUILDING doc to encourage parallelizing make. When I
first built node I didn't know this trick and thought that the build was
just stuck in an infinite loop after waiting for 10 minutes.

Refs: #8286
Refs: #9881

PR-URL: #9961
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
MylesBorins pushed a commit that referenced this pull request Jan 16, 2017
Standardizes docs to use -j4 instead of -j8 as it appears to be the
most inclusive recommendation based on discussion in
#9961.

PR-URL: #9961
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
@MylesBorins
Copy link
Contributor

the v4.x building.md does not include any information regarding parallel make. Please feel free to backport.

MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
Adds a note to the BUILDING doc to encourage parallelizing make. When I
first built node I didn't know this trick and thought that the build was
just stuck in an infinite loop after waiting for 10 minutes.

Refs: #8286
Refs: #9881

PR-URL: #9961
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
Standardizes docs to use -j4 instead of -j8 as it appears to be the
most inclusive recommendation based on discussion in
#9961.

PR-URL: #9961
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2017
MylesBorins pushed a commit that referenced this pull request Jan 31, 2017
Adds a note to the BUILDING doc to encourage parallelizing make. When I
first built node I didn't know this trick and thought that the build was
just stuck in an infinite loop after waiting for 10 minutes.

Refs: #8286
Refs: #9881

PR-URL: #9961
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
MylesBorins pushed a commit that referenced this pull request Jan 31, 2017
Standardizes docs to use -j4 instead of -j8 as it appears to be the
most inclusive recommendation based on discussion in
#9961.

PR-URL: #9961
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet