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

VM: VM is much slower when timeout was specified #10453

Closed
mooyoul opened this issue Dec 25, 2016 · 2 comments
Closed

VM: VM is much slower when timeout was specified #10453

mooyoul opened this issue Dec 25, 2016 · 2 comments
Labels
doc Issues and PRs related to the documentations. performance Issues and PRs related to the performance of Node.js. question Issues that look for answers. vm Issues and PRs related to the vm subsystem.

Comments

@mooyoul
Copy link

mooyoul commented Dec 25, 2016

  • Version:
    Node.js v6.7.0 / Node.js 4.4.2
  • Platform:
    Ubuntu 14.04 LTS x64 (v6.7.0) / macOS Sierra (v4.4.2)
  • Subsystem:
    VM

2016-12-26 4 23 20

I just found there's unexpected behavior on timeout option of vm.runInContext.

When timeout option was specified, Script execution on VM is much slower then timeout options wasn't specified.

This is two of example snippets to reproduce that problem:

vm-normal.js
'use strict';

const
  vm = require('vm'),
  sandbox = {
    current: -1
  };

console.time('vm-normal');

vm.createContext(sandbox);

for (let i = 0 ; i < 100000 ; i++) {
  vm.runInContext(`current = ${i};`, sandbox, {
    displayErrors: false
  });
}

console.timeEnd('vm-normal');
vm-with-timeout.js
'use strict';

const
  vm = require('vm'),
  sandbox = {
    current: -1
  };

console.time('vm-with-timeout');

vm.createContext(sandbox);

for (let i = 0 ; i < 100000 ; i++) {
  vm.runInContext(`current = ${i};`, sandbox, {
    displayErrors: false,
    timeout: 1000
  });
}

console.timeEnd('vm-with-timeout');

I think maybe that's not a bug (timer for timeout is expansive?), but that's unexpected behavior. It would be nice if that limitation should be documented on VM API documentation.

@addaleax addaleax added performance Issues and PRs related to the performance of Node.js. vm Issues and PRs related to the vm subsystem. labels Dec 25, 2016
@addaleax
Copy link
Member

addaleax commented Dec 25, 2016

When timeout option was specified, Script execution on VM is much slower then timeout options wasn't specified.

The script execution isn’t slower, but of course there’s overhead associated with setting up the timer and stopping it again (especially because Node can’t use its usual facilities for that), which happens once for each runInContext call.

You can move the for loop into the script itself and the difference should vanish. You can feel free to open a PR against the documentation, of course.

@addaleax addaleax added the doc Issues and PRs related to the documentations. label Dec 25, 2016
@mscdex
Copy link
Contributor

mscdex commented Dec 25, 2016

Also on an unrelated note, using the let in the for like that is slower (not sure about node v7).

@addaleax addaleax added the question Issues that look for answers. label Apr 29, 2017
addaleax added a commit to addaleax/node that referenced this issue Apr 29, 2017
Mention that the `timeout` option has a noticeable performance impact.

Fixes: nodejs#10453
anchnk pushed a commit to anchnk/node that referenced this issue 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]>
gibfahn pushed a commit that referenced this issue 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 issue 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 issue 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. performance Issues and PRs related to the performance of Node.js. question Issues that look for answers. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants