doc: adding estimated execution time #41142

mawaregetsuka · 2021-12-11T18:13:47Z

adding estimated execution times is more firendly for new contributer

doc/guides/contributing/pull-requests.md

DerekNonGeneric

If I am not mistaken, there is a way to increase the speed of these commands by specifying that you would like to use more processor cores, but I don't remember what that command is or whether it is documented anywhere. If we don't have that neat trick documented anywhere, I think it would be good to do so.

Would anyone happen to know if/what option(s) to pass to increase this speed?

/cc @devsnek @codebytere 🙏

Trott · 2021-12-12T13:40:17Z

doc/guides/contributing/pull-requests.md

+(This code might take more than an hour to execute on a PC, want to run a part
+of tests quickly or for more details see the [running tests][] section of
+Building guide.)


Suggested change

(This code might take more than an hour to execute on a PC, want to run a part

of tests quickly or for more details see the [running tests][] section of

Building guide.)

Running tests can take a long time. To run a subset of the test suite, see the

[running tests][] section of the Building guide.

Agree with you there is no need for too much explanation here. I used the -j4 argument at run time, but it still takes more than an hour.So I thought it might be a good idea to add a numerical estimated time.

Please tell me what you think and finish the PR
@Trott

I don't know what's the typical experience (and I'm not sure that anyone else does either) so I'm inclined to not try to specify a time and go with the suggestion I made above. However, if we think it would be helpful to include a running time, maybe something like this?: "Depending on your hardware, running tests might take an hour or more."

Of course I understand that time is hardware dependent, but based on my tests I don't think I can achieve results in less than an hour without a specialized compilation computer.
"Depending on your hardware, running tests might take an hour or more."maybe a good answer

On my Windows PC, it takes less than 15 minutes to compile and test.

I exceeded more than an hour on both my Windows PC and Linux laptops and the laptop took a little longer. Maybe my hardware is too old?

How about using build time in CI as the standard?

I think the text as it currently stands sufficiently covers things:

For some configurations, running all tests might take a long time (an hour or
more). To run a subset of the test suite, see the [running tests][] section of
the Building guide.

If that works for you, this can land. We have all the right approvals and CI is green.

No problem. I think the current version is pretty good

Trott · 2021-12-12T13:44:55Z

Would anyone happen to know if/what option(s) to pass to increase this speed?

Not sure about vcbuild but on POSIX, building/compiling can be sped up with the -j <number> option. It's already in this doc as -j 4. It's in the BUILDING.md doc. But it's hard to explain, easy to get wrong, etc. So we tend to just leave it as -j 4 in commands and don't bother distracting people who just want to open a pull request with issues like how it doesn't really matter for the test suite (only the compilation running up to the test suite), how you may be tempted to run it without the <number> part but you'll just fork-bomb yourself so please don't, how you can't really know if you should use -j 4 or -j 2 or -j 8 or whatever but you can sorta guess that using the same number of cores that you have available is likely to be about right, how this won't do a darned thing probably for people with a single core, and so on.

As you can see by the bad job I've done explaining it here, it's easy to write a ton of text that most people will never need and is hard to understand. If we do end up going into detail on this, it should probably go inside a <details> tag.

I've been on-and-off trying to get pre-compilation working with GitPod so new contributors don't have to compile at all, but I keep not following through.

DerekNonGeneric · 2021-12-16T04:44:59Z

I just found my old Node Core notes that I took on the subject nearly a year ago…

make -j{number of logical cores}

of CPU Cores 4
# of Threads 8

make -j8

These instructions would not be sufficient w/o first including the prerequisite that the person building Node first runs the lscpu command (if on a ~Linux OS) to first review the number of the available logical CPUs…

In this example 8 logical CPUs are available, 4 physical cores each with 2 threads, hence one could set the MAKEOPTS variable to:

MAKEOPTS="-j8"

Refs: https://wiki.gentoo.org/wiki/MAKEOPTS

We should probably document this, but probably in a separate PR.

mawaregetsuka · 2021-12-16T07:20:31Z

@DerekNonGeneric
good idea!
it's remind contributors to set parameters based on the number of CPU threads

targos · 2021-12-16T12:33:51Z

Not sure about vcbuild

On Windows, the maximum number of parallelisation is used by default (which can make the system unresponsive sometimes). I don't remember how to change this.

doc/guides/contributing/pull-requests.md

tniessen · 2021-12-16T15:36:16Z

FWIW, building and testing debug (non-release) builds can take a long time, and -j commonly crashes the linker ld on systems with less than 16 GiB of memory or so because make runs multiple memory-intensive operations at the same time, and debug builds unfortunately require a ton of memory.

nodejs-github-bot · 2021-12-17T19:18:22Z

Landed in e676942

PR-URL: #41142 Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Stewart X Addison <sxa@redhat.com>

PR-URL: nodejs#41142 Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Stewart X Addison <sxa@redhat.com>

PR-URL: #41142 Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Stewart X Addison <sxa@redhat.com>

Siddhant-K-code · 2022-06-01T15:20:14Z

I've been on-and-off trying to get pre-compilation working with GitPod so new contributors don't have to compile at all, but I keep not following through.

👋🏼 @Trott

We can give it a try, for now, it works perfectly fine.

F3n67u · 2022-06-11T12:00:33Z

I've been on-and-off trying to get pre-compilation working with GitPod so new contributors don't have to compile at all, but I keep not following through.

👋🏼 @Trott

We can give it a try, for now, it works perfectly fine.

Gitpod prebuild is great. I use gitpod a lot when I develop node core. It provides 16 core and 64 memory out of the box.
I will big +1 if node will consider add gitpod prebuild support.

Trott · 2022-06-11T15:03:12Z

Gitpod prebuild is great. I use gitpod a lot when I develop node core. It provides 16 core and 64 memory out of the box.
I will big +1 if node will consider add gitpod prebuild support.

I'd be curious what your .gitpod.yml file or other settings are. I have to do something mildly unintuitive to make it work and I wonder if I'm Doing It Wrong™,

F3n67u · 2022-06-12T06:59:08Z

Gitpod prebuild is great. I use gitpod a lot when I develop node core. It provides 16 core and 64 memory out of the box.
I will big +1 if node will consider add gitpod prebuild support.

I'd be curious what your .gitpod.yml file or other settings are. I have to do something mildly unintuitive to make it work and I wonder if I'm Doing It Wrong™,

I dont have any .gitpod.yml and other setting. gitpod has almost everything I need to build node: gcc, clang, make, even ninja is also included.

nodejs-github-bot added the doc Issues and PRs related to the documentations. label Dec 11, 2021

mawaregetsuka force-pushed the master branch from 6eef97f to 3b18d86 Compare December 11, 2021 18:57

doc: adding estimated execution time

3b18d86

DerekNonGeneric reviewed Dec 11, 2021

View reviewed changes

doc/guides/contributing/pull-requests.md Outdated Show resolved Hide resolved

fixup! doc: adding estimated execution time

48ae57d

DerekNonGeneric approved these changes Dec 11, 2021

View reviewed changes

Trott reviewed Dec 12, 2021

View reviewed changes

Update doc/guides/contributing/pull-requests.md

6c6957f

Trott reviewed Dec 16, 2021

View reviewed changes

doc/guides/contributing/pull-requests.md Outdated Show resolved Hide resolved

Update doc/guides/contributing/pull-requests.md

fcf00da

Trott added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Dec 16, 2021

Trott approved these changes Dec 16, 2021

View reviewed changes

Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 16, 2021

tniessen approved these changes Dec 16, 2021

View reviewed changes

sxa approved these changes Dec 17, 2021

View reviewed changes

Trott added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 17, 2021

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 17, 2021

nodejs-github-bot merged commit e676942 into nodejs:master Dec 17, 2021

targos mentioned this pull request Jan 16, 2022

v17.4.0 release proposal #41557

Merged

danielleadams mentioned this pull request Feb 1, 2022

v16.14.0 proposal #41804

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: adding estimated execution time #41142

doc: adding estimated execution time #41142

mawaregetsuka commented Dec 11, 2021

DerekNonGeneric left a comment •

edited

Loading

Trott Dec 12, 2021

mawaregetsuka Dec 12, 2021

mawaregetsuka Dec 14, 2021

Trott Dec 15, 2021 •

edited

Loading

mawaregetsuka Dec 16, 2021

targos Dec 16, 2021

mawaregetsuka Dec 16, 2021

mawaregetsuka Dec 16, 2021

Trott Dec 16, 2021

mawaregetsuka Dec 17, 2021

Trott commented Dec 12, 2021 •

edited

Loading

DerekNonGeneric commented Dec 16, 2021 •

edited

Loading

mawaregetsuka commented Dec 16, 2021

targos commented Dec 16, 2021

tniessen commented Dec 16, 2021

nodejs-github-bot commented Dec 17, 2021

Siddhant-K-code commented Jun 1, 2022

F3n67u commented Jun 11, 2022

Trott commented Jun 11, 2022

F3n67u commented Jun 12, 2022 •

edited

Loading

doc: adding estimated execution time #41142

doc: adding estimated execution time #41142

Conversation

mawaregetsuka commented Dec 11, 2021

DerekNonGeneric left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott Dec 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott commented Dec 12, 2021 • edited Loading

DerekNonGeneric commented Dec 16, 2021 • edited Loading

mawaregetsuka commented Dec 16, 2021

targos commented Dec 16, 2021

tniessen commented Dec 16, 2021

nodejs-github-bot commented Dec 17, 2021

Siddhant-K-code commented Jun 1, 2022

F3n67u commented Jun 11, 2022

Trott commented Jun 11, 2022

F3n67u commented Jun 12, 2022 • edited Loading

DerekNonGeneric left a comment •

edited

Loading

Trott Dec 15, 2021 •

edited

Loading

Trott commented Dec 12, 2021 •

edited

Loading

DerekNonGeneric commented Dec 16, 2021 •

edited

Loading

F3n67u commented Jun 12, 2022 •

edited

Loading