Skip to content

Conversation

@addaleax
Copy link
Member

Mention that the timeout option has a noticeable performance impact.

Fixes: #10453

Checklist
Affected core subsystem(s)

doc, vm

Mention that the `timeout` option has a noticeable performance impact. Fixes: nodejs#10453
@nodejs-github-botnodejs-github-bot added doc Issues and PRs related to the documentations. vm Issues and PRs related to the vm subsystem. labels Apr 29, 2017
```

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

Choose a reason for hiding this comment

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

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

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

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. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

@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.)

Copy link
Contributor

@refackrefackApr 29, 2017

Choose a reason for hiding this comment

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

Maybe remove non-zero

Copy link
Member

@mhdawsonmhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@mhdawson
Copy link
Member

@addaleax
Copy link
MemberAuthor

Landed in 2bf461e

@addaleaxaddaleax closed this May 3, 2017
@addaleaxaddaleax deleted the doc-vm-timeout-perf branch May 3, 2017 13:30
addaleax added a commit that referenced this pull request May 3, 2017
Mention that the `timeout` option has a noticeable performance impact. Fixes: #10453 PR-URL: #12751 Reviewed-By: Alexey Orlenko <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
anchnk pushed a commit to anchnk/node that referenced this pull request May 6, 2017
Mention that the `timeout` option has a noticeable performance impact. Fixes: nodejs#10453 PR-URL: nodejs#12751 Reviewed-By: Alexey Orlenko <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
@jasnelljasnell mentioned this pull request May 11, 2017
@gibfahngibfahn mentioned this pull request Jun 15, 2017
3 tasks
gibfahn pushed a commit that referenced this pull request Jun 18, 2017
Mention that the `timeout` option has a noticeable performance impact. Fixes: #10453 PR-URL: #12751 Reviewed-By: Alexey Orlenko <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
gibfahn pushed a commit that referenced this pull request Jun 20, 2017
Mention that the `timeout` option has a noticeable performance impact. Fixes: #10453 PR-URL: #12751 Reviewed-By: Alexey Orlenko <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2017
Mention that the `timeout` option has a noticeable performance impact. Fixes: #10453 PR-URL: #12751 Reviewed-By: Alexey Orlenko <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Jul 18, 2017
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docIssues and PRs related to the documentations.vmIssues and PRs related to the vm subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

VM: VM is much slower when timeout was specified

7 participants

@addaleax@mhdawson@refack@jasnell@aqrln@gibfahn@nodejs-github-bot