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: document vm timeout option perf impact #12751

Closed
wants to merge 1 commit into from

Conversation

@addaleax
Copy link
Member

commented Apr 29, 2017

Mention that the timeout option has a noticeable performance impact.

Fixes: #10453

Checklist
Affected core subsystem(s)

doc, vm

doc: document vm timeout option perf impact
Mention that the `timeout` option has a noticeable performance impact.

Fixes: #10453
@@ -125,6 +125,10 @@ console.log(util.inspect(sandbox));
// { animal: 'cat', count: 12, name: 'kitty' }
```

*Note*: Using the `timeout` or `breakOnSigint` options will result in new
event loops and corresponding threads being started, which have a non-zero

This comment has been minimized.

Copy link
@aqrln

aqrln Apr 29, 2017

Member

Does using breakOnSigint option start a new event loop though? Either I am wrong or the language is somewhat confusing here.

This comment has been minimized.

Copy link
@addaleax

addaleax Apr 29, 2017

Author Member

Yeah, breakOnSigint doesn’t start its own loop, only an extra thread. It’s definitely just an implementation detail, but if you have suggestions for better wording, I’ll take them. :)

This comment has been minimized.

Copy link
@aqrln

aqrln Apr 29, 2017

Member

I'd say something like "Using the timeout option will result in creating a new event loop and utilizing the breakOnSigint option will cause a helper thread to be started", but, well, I agree that it is an implementation detail and your version is more concise, so I'm fine with it either way.

This comment has been minimized.

Copy link
@addaleax

addaleax Apr 29, 2017

Author Member

@aqrln The timeout option will create both a thread and a new event loop. ;) (I would actually assume spinning up the thread is much more costly – basically, what I’m trying to say is that both will allocate and consume new resources.)

This comment has been minimized.

Copy link
@refack

refack Apr 29, 2017

Member

Maybe remove non-zero

@aqrln
aqrln approved these changes Apr 29, 2017
@jasnell
jasnell approved these changes May 1, 2017
@mhdawson
Copy link
Member

left a comment

LGTM

@mhdawson

This comment has been minimized.

Copy link
Member

commented May 1, 2017

@addaleax

This comment has been minimized.

Copy link
Member Author

commented May 3, 2017

Landed in 2bf461e

@addaleax addaleax closed this May 3, 2017

@addaleax addaleax deleted the addaleax:doc-vm-timeout-perf branch May 3, 2017

addaleax added a commit that referenced this pull request May 3, 2017
doc: document vm timeout option perf impact
Mention that the `timeout` option has a noticeable performance impact.

Fixes: #10453
PR-URL: #12751
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
anchnk pushed a commit to anchnk/node that referenced this pull request May 6, 2017
doc: document vm timeout option perf impact
Mention that the `timeout` option has a noticeable performance impact.

Fixes: nodejs#10453
PR-URL: nodejs#12751
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@jasnell jasnell referenced this pull request May 11, 2017
@gibfahn gibfahn referenced this pull request Jun 15, 2017
2 of 3 tasks complete
gibfahn added a commit that referenced this pull request Jun 18, 2017
doc: document vm timeout option perf impact
Mention that the `timeout` option has a noticeable performance impact.

Fixes: #10453
PR-URL: #12751
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
gibfahn added a commit that referenced this pull request Jun 20, 2017
doc: document vm timeout option perf impact
Mention that the `timeout` option has a noticeable performance impact.

Fixes: #10453
PR-URL: #12751
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins added a commit that referenced this pull request Jul 11, 2017
doc: document vm timeout option perf impact
Mention that the `timeout` option has a noticeable performance impact.

Fixes: #10453
PR-URL: #12751
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@MylesBorins MylesBorins referenced this pull request Jul 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.