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: adding estimated execution time #41142

Merged
merged 4 commits into from
Dec 17, 2021

Conversation

mawaregetsuka
Copy link
Contributor

adding estimated execution times is more firendly for new contributer

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

@DerekNonGeneric DerekNonGeneric left a comment

Choose a reason for hiding this comment

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

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 🙏

Comment on lines 254 to 256
(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.)
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@Trott Trott Dec 15, 2021

Choose a reason for hiding this comment

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

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."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn 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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about using build time in CI as the standard?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem. I think the current version is pretty good

@Trott
Copy link
Member

Trott commented Dec 12, 2021

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
Copy link
Contributor

DerekNonGeneric commented Dec 16, 2021

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
Copy link
Contributor Author

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

@targos
Copy link
Member

targos commented Dec 16, 2021

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.

@Trott 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 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
Copy link
Member

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.

@Trott Trott added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 17, 2021
@nodejs-github-bot 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 nodejs-github-bot merged commit e676942 into nodejs:master Dec 17, 2021
@nodejs-github-bot
Copy link
Collaborator

Landed in e676942

targos pushed a commit that referenced this pull request Jan 14, 2022
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>
danielleadams pushed a commit that referenced this pull request Jan 31, 2022
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>
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
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>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
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>
@danielleadams danielleadams mentioned this pull request Feb 1, 2022
@Siddhant-K-code
Copy link

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
Copy link
Member

F3n67u commented Jun 11, 2022

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
Copy link
Member

Trott commented Jun 11, 2022

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
Copy link
Member

F3n67u commented Jun 12, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants