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 useful V8 option section #32262

Closed
wants to merge 9 commits into from
Closed

Conversation

nimit95
Copy link
Contributor

@nimit95 nimit95 commented Mar 14, 2020

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

nimit95 added 2 commits March 14, 2020 22:16
This adds new section for v8 options and --max-old-space-size
Fixes: nodejs#32252
Fixed a typo addition in doc/cli.md
Fixes: nodejs#32252
@nodejs-github-bot nodejs-github-bot added cli Issues and PRs related to the Node.js command line interface. doc Issues and PRs related to the documentations. labels Mar 14, 2020
doc/api/cli.md Outdated Show resolved Hide resolved
doc/api/cli.md Outdated Show resolved Hide resolved
doc/api/cli.md Outdated

These are v8 options and fall outside Node.js' responsibility.

V8 options that are allowed are:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should either be removed completely or at least reworded. Node does not restrict the V8 options that can be passed on the command line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is another place in the document mentioning the same thing.

doc/api/cli.md Outdated
Comment on lines 1396 to 1398
Sets the max memory size of V8 old memory section. As memory consumption approaches the limit, V8 will spend more time on garbage collection in an effort to free unused memory.

On a machine with 2GB of memory it suggested to set 1.5GB to leave some memory for other uses and avoid swapping.
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
Sets the max memory size of V8 old memory section. As memory consumption approaches the limit, V8 will spend more time on garbage collection in an effort to free unused memory.
On a machine with 2GB of memory it suggested to set 1.5GB to leave some memory for other uses and avoid swapping.
Sets the max memory size of V8 old memory section. As memory
consumption approaches the limit, V8 will spend more time on
garbage collection in an effort to free unused memory.
On a machine with 2GB of memory it suggested to set 1.5GB to
leave some memory for other uses and avoid swapping.

nit: need to wrap lines at <= 80 chars

doc/api/cli.md Outdated
@@ -1385,6 +1385,20 @@ threadpool by setting the `'UV_THREADPOOL_SIZE'` environment variable to a value
greater than `4` (its current default value). For more information, see the
[libuv threadpool documentation][].

## Useful v8 options

These are v8 options and fall outside Node.js' responsibility.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
These are v8 options and fall outside Node.js' responsibility.
V8 has CLI options. Any V8 CLI option that is provided to `node` will be passed on to V8 to handle. V8's options have _no stability guarantee_. The V8 team themselves don't consider them to be part of their formal API, and reserve the right to change them at any time. The Node.js team does not consider them covered by the Node.js stability guarantees. Many of the V8 options are of interest only to V8 developers. Despite this, there is a small set of V8 options that are widely applicable to Node.js, and they are documented here.

I think we should say something like the above.

doc/api/cli.md Outdated

V8 options that are allowed are:

### `--max-old-space-size`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
### `--max-old-space-size`
### `--max-old-space-size=SIZE` (in Mbytes)

@nimit95 nimit95 changed the title Doc: Add useful v8 option section doc: Add useful v8 option section Mar 17, 2020
nimit95 and others added 2 commits March 17, 2020 11:22
Co-Authored-By: mscdex <mscdex@users.noreply.github.com>
Co-Authored-By: mscdex <mscdex@users.noreply.github.com>
@HarshithaKP
Copy link
Member

according to me, explaning the gc behavior is not necessary while describing the heap size flag.

also, elaborate description about API stability can confuse people?

how about just this:
size of the javascript heap (tenured heap or old space), in MBs. (V8 options are not processed by Node.js, and are passed as is to V8)

@nimit95
Copy link
Contributor Author

nimit95 commented Mar 17, 2020

Detailed about V8 because people can get confused about the behaviour of GC, we are explicitly mentioning that it is managed by V8 here. Also, it doesn't lie in node preview.

Signed-off-by: Nimit <nimitagg95@gmail.com>
doc/api/cli.md Outdated
and reserve the right to change them at any time. The Node.js team does not
consider them covered by the Node.js stability guarantees. Many of the V8
options are of interest only to V8 developers. Despite this, there is a small
set of V8 options that are widely applicable to the Node.js, and they are
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
set of V8 options that are widely applicable to the Node.js, and they are
set of V8 options that are widely applicable to Node.js, and they are

doc/api/cli.md Outdated
consider them covered by the Node.js stability guarantees. Many of the V8
options are of interest only to V8 developers. Despite this, there is a small
set of V8 options that are widely applicable to the Node.js, and they are
documented here. V8 options that are allowed are:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
documented here. V8 options that are allowed are:
documented here:

doc/api/cli.md Outdated
consumption approaches the limit, V8 will spend more time on
garbage collection in an effort to free unused memory.

On a machine with 2GB of memory it suggested to set 1.5GB
Copy link
Contributor

@mscdex mscdex Mar 17, 2020

Choose a reason for hiding this comment

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

Suggested change
On a machine with 2GB of memory it suggested to set 1.5GB
On a machine with 2GB of memory it is suggested to set this to 1536 (1.5GB)

doc/api/cli.md Outdated

### `--max-old-space-size=SIZE` (in Mbytes)

Sets the max memory size of V8 old memory section. As memory
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Sets the max memory size of V8 old memory section. As memory
Sets the max memory size of V8's old memory section. As memory

Signed-off-by: Nimit <nimitagg95@gmail.com>
@mscdex
Copy link
Contributor

mscdex commented Mar 17, 2020

Just a note: when squashing/landing, make sure 'V8' is used instead of 'v8' in the commit message to avoid confusion.

@nimit95 nimit95 changed the title doc: Add useful v8 option section doc: Add useful V8 option section Mar 18, 2020
@nimit95
Copy link
Contributor Author

nimit95 commented Mar 24, 2020

Anything to be done here from me?

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 29, 2020
doc/api/cli.md Outdated
Comment on lines 1393 to 1394
and reserve the right to change them at any time. The Node.js team does not
consider them covered by the Node.js stability guarantees. Many of the V8
Copy link
Member

Choose a reason for hiding this comment

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

Optional nit:

Suggested change
and reserve the right to change them at any time. The Node.js team does not
consider them covered by the Node.js stability guarantees. Many of the V8
and reserve the right to change them at any time. Likewise, they are not
covered by the Node.js stability guarantees. Many of the V8

doc/api/cli.md Outdated
consumption approaches the limit, V8 will spend more time on
garbage collection in an effort to free unused memory.

On a machine with 2GB of memory it is suggested to set this to
Copy link
Member

@Trott Trott Mar 30, 2020

Choose a reason for hiding this comment

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

Missing comma:

Suggested change
On a machine with 2GB of memory it is suggested to set this to
On a machine with 2GB of memory, it is suggested to set this to

Optional nit: I'm not a fan of "it is suggested" or "it is recommended" formulations. If possible, either tell people to do something or don't tell them at all. If you can't make a blanket statement, it might at least be a little more direct with something like "consider":

Suggested change
On a machine with 2GB of memory it is suggested to set this to
On a machine with 2GB of memory, consider setting this to

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understood.

doc/api/cli.md Outdated
On a machine with 2GB of memory it is suggested to set this to
1536 (1.5GB) to leave some memory for other uses and avoid swapping.

E.g. `node --max-old-space-size=1536 index.js`
Copy link
Member

Choose a reason for hiding this comment

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

Optional nit: Delete this line.

Copy link
Contributor Author

@nimit95 nimit95 Apr 3, 2020

Choose a reason for hiding this comment

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

I personally feel an example helps a lot when being introduced to a flag. It clearly shows how to pass an argument. @Trott

Copy link
Member

Choose a reason for hiding this comment

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

I personally feel an example helps a lot when being introduced to a flag

If we want to provide an example, it should conform with the formatting of the other examples provided elsewhere in the doc. Otherwise, each entry is inventing its own example formatting, and we definitely do not want that. The existing format is more useful than this as it provides context, such as what the anticipated output might look like. So, this example might show the error one might get if running without the flag, then show the command succeeding with the flag.

(That said, most of our entries do not contain examples and that seems appropriate in this document.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Totally agree with your point, Can you point me to some of the correct existing formats?
What I see a mixture of formats, some given below

  1. node --name-of-flag index.mjs
  2. node --name-of-flag

Copy link
Member

Choose a reason for hiding this comment

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

I don't mean the format of the command itself. I mean the markdown formatting that denotes an example.

```console
$ node --cpu-prof index.js
$ ls *.cpuprofile
CPU.20190409.202950.15293.0.0.cpuprofile
```

Copy link
Contributor Author

@nimit95 nimit95 Apr 8, 2020

Choose a reason for hiding this comment

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

@Trott can you check out the formatting once now.

@addaleax
Copy link
Member

addaleax commented Apr 2, 2020

@Trott do you want to apply your suggestions and merge this?

@nimit95
Copy link
Contributor Author

nimit95 commented Apr 2, 2020

Hey, will make the changes today. Totally forgot about this :)

@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 2, 2020
Signed-off-by: Nimit <nimitagg95@gmail.com>
@Trott
Copy link
Member

Trott commented Apr 5, 2020

My one remaining comment is a nit. In other words, it expresses my personal opinion, but I don't feel so strongly about it that I would block landing this if others disagree with my view. So if @addaleax or anyone else thinks this should land now as it is, go for it.

Signed-off-by: Nimit <nimitagg95@gmail.com>
gireeshpunathil pushed a commit that referenced this pull request Apr 11, 2020
This adds new section for v8 options and --max-old-space-size
Fixes: #32252

PR-URL: #32262
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
@gireeshpunathil
Copy link
Member

landed in df05b07

targos pushed a commit that referenced this pull request Apr 13, 2020
This adds new section for v8 options and --max-old-space-size
Fixes: #32252

PR-URL: #32262
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
BethGriggs pushed a commit that referenced this pull request Apr 14, 2020
This adds new section for v8 options and --max-old-space-size
Fixes: #32252

PR-URL: #32262
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
targos pushed a commit that referenced this pull request Apr 22, 2020
This adds new section for v8 options and --max-old-space-size
Fixes: #32252

PR-URL: #32262
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli Issues and PRs related to the Node.js command line interface. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants