vm: improve performance of vm.runIn*() #10816

Trott · 2017-01-15T06:49:46Z

Optimize for common cases in vm.runInContext() and vm.runInThisContext() when breakOnSigint is set.

$ node benchmark/compare.js --set 'n=10'  --old './node-old' --new './node-new' vm > bench.csv
$ cat bench.csv | Rscript benchmark/compare.R bench
                                                                     improvement confidence      p.value
 vm/run-in-context.js withSigintListener=0 breakOnSigint=0 n=10           1.12 %            0.5268470825
 vm/run-in-context.js withSigintListener=0 breakOnSigint=1 n=10           6.90 %         ** 0.0039743927
 vm/run-in-context.js withSigintListener=1 breakOnSigint=0 n=10           5.19 %        *** 0.0001940968
 vm/run-in-context.js withSigintListener=1 breakOnSigint=1 n=10          -0.84 %            0.6936019893
 vm/run-in-this-context.js withSigintListener=0 breakOnSigint=0 n=10      0.14 %            0.9651596335
 vm/run-in-this-context.js withSigintListener=0 breakOnSigint=1 n=10      9.27 %         ** 0.0058309681
 vm/run-in-this-context.js withSigintListener=1 breakOnSigint=0 n=10      1.13 %            0.5957890149
 vm/run-in-this-context.js withSigintListener=1 breakOnSigint=1 n=10      2.21 %            0.2564659136
$

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

vm benchmark

mscdex · 2017-01-15T08:11:51Z

lib/vm.js

-  if (!Array.isArray(sigintListeners))
-    sigintListeners = sigintListeners ? [sigintListeners] : [];
-  else
+  if (!sigintListeners)


What about moving this to the conditional in the caller function (options && options.breakOnSigint) to avoid creating the argsArray and calling fn.apply()?

Introduce benchmarks for vm.runInContext() and vm.runInThisContext().

Optimize for common cases in vm.runInContext() and vm.runInThisContext().

Trott · 2017-01-15T17:14:21Z

With the change suggested by @mscdex:

$  node benchmark/compare.js --set 'n=10'  --old './node-old' --new './node-new' vm > bench.csv
$ cat bench.csv | Rscript benchmark/compare.R bench
                                                                     improvement confidence      p.value
 vm/run-in-context.js withSigintListener=0 breakOnSigint=0 n=10           1.69 %            0.4951121692
 vm/run-in-context.js withSigintListener=0 breakOnSigint=1 n=10           7.41 %         ** 0.0083586822
 vm/run-in-context.js withSigintListener=1 breakOnSigint=0 n=10           6.29 %        *** 0.0000524146
 vm/run-in-context.js withSigintListener=1 breakOnSigint=1 n=10           1.23 %            0.4899217968
 vm/run-in-this-context.js withSigintListener=0 breakOnSigint=0 n=10     -1.33 %            0.1885065483
 vm/run-in-this-context.js withSigintListener=0 breakOnSigint=1 n=10      7.24 %          * 0.0306398665
 vm/run-in-this-context.js withSigintListener=1 breakOnSigint=0 n=10     -2.97 %            0.3052677075
 vm/run-in-this-context.js withSigintListener=1 breakOnSigint=1 n=10     -0.01 %            0.9979790584
$

mscdex · 2017-01-15T22:50:56Z

LGTM.

FWIW here's the results I got when testing locally with a larger number of iterations:

                                                                         improvement confidence      p.value
 vm/run-in-context.js withSigintListener=0 breakOnSigint=0 n=700000           0.92 %            1.160369e-01
 vm/run-in-context.js withSigintListener=0 breakOnSigint=1 n=700000          12.29 %        *** 1.250555e-27
 vm/run-in-context.js withSigintListener=1 breakOnSigint=0 n=700000           2.14 %        *** 1.608620e-04
 vm/run-in-context.js withSigintListener=1 breakOnSigint=1 n=700000           2.22 %        *** 9.483438e-07
 vm/run-in-this-context.js withSigintListener=0 breakOnSigint=0 n=700000      0.70 %            4.120226e-01
 vm/run-in-this-context.js withSigintListener=0 breakOnSigint=1 n=700000     12.15 %        *** 5.852368e-19
 vm/run-in-this-context.js withSigintListener=1 breakOnSigint=0 n=700000     -0.22 %            7.966101e-01
 vm/run-in-this-context.js withSigintListener=1 breakOnSigint=1 n=700000      0.64 %            2.948688e-01

Trott · 2017-01-15T23:39:11Z

@mscdex Is that with a large n or by providing a large number for the --runs option or something else?

mscdex · 2017-01-16T01:33:09Z

@Trott I left the --runs at the default (currently 30) and added --set n=700000 to the comparison command line.

bnoordhuis

LGTM with some questions.

bnoordhuis · 2017-01-16T04:12:00Z

benchmark/vm/run-in-context.js

+  if (withSigintListener)
+    process.on('SIGINT', () => {});
+
+  var i = 0;


Out of curiosity, is there a reason you define i here and not in the for block where it's used?

I used crypto/get-ciphers.js as a template and that is what is done there. I have no strong opinions on it so I left it outside the block.

bnoordhuis · 2017-01-16T04:13:17Z

benchmark/vm/run-in-context.js

+const common = require('../common.js');
+
+const bench = common.createBenchmark(main, {
+  n: [1],


Just a single iteration?

Yeah, I had to bump up n considerably to get the benchmarks to run long enough for me to be comfortable with the results, but then again I find myself having to do that a lot with many of the other existing benchmarks ...

Trott · 2017-01-17T06:19:43Z

CI: https://ci.nodejs.org/job/node-test-pull-request/5904/

Introduce benchmarks for vm.runInContext() and vm.runInThisContext(). PR-URL: #10816 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Brian White <[email protected]>

Optimize for common cases in vm.runInContext() and vm.runInThisContext(). PR-URL: #10816 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Brian White <[email protected]>

jasnell · 2017-01-18T16:29:33Z

Landed in 030dd14 and ef1e77d

Introduce benchmarks for vm.runInContext() and vm.runInThisContext(). PR-URL: nodejs#10816 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Brian White <[email protected]>

Optimize for common cases in vm.runInContext() and vm.runInThisContext(). PR-URL: nodejs#10816 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Brian White <[email protected]>

Introduce benchmarks for vm.runInContext() and vm.runInThisContext(). PR-URL: nodejs#10816 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Brian White <[email protected]>

Optimize for common cases in vm.runInContext() and vm.runInThisContext(). PR-URL: nodejs#10816 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Brian White <[email protected]>

Introduce benchmarks for vm.runInContext() and vm.runInThisContext(). PR-URL: nodejs#10816 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Brian White <[email protected]>

Optimize for common cases in vm.runInContext() and vm.runInThisContext(). PR-URL: nodejs#10816 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Brian White <[email protected]>

Introduce benchmarks for vm.runInContext() and vm.runInThisContext(). PR-URL: nodejs#10816 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Brian White <[email protected]>

Optimize for common cases in vm.runInContext() and vm.runInThisContext(). PR-URL: nodejs#10816 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Brian White <[email protected]>

MylesBorins · 2017-03-08T02:11:00Z

lts???

MylesBorins · 2017-05-08T15:12:49Z

ping re: LTS

Trott · 2017-05-08T22:17:04Z

Since it's a perf-based refactor and not a behavior change, I think landing on 6.x is appropriate.

MylesBorins · 2017-05-15T15:26:22Z

This does not land cleanly in LTS. Would someone be able to backport?

Trott · 2017-05-15T19:11:35Z

@MylesBorins This will land cleanly after #9388 lands on v6.x-staging.

Trott · 2017-05-15T19:12:11Z

Removing the backport-requested-v6.x label as I think landing that other PR first is probably the better way to go. Feel free to re-add the label if you disagree, of course.

MylesBorins · 2017-05-16T05:23:30Z

@Trott fwiw I doubt the naming anonymous functions is going to be backported for a couple releases

gibfahn · 2017-06-17T20:05:46Z

Adding baking-for-lts, as this depends on #9388 which also has that label.

Trott added benchmark Issues and PRs related to the benchmark subsystem. vm Issues and PRs related to the vm subsystem. labels Jan 15, 2017

mscdex reviewed Jan 15, 2017

View reviewed changes

Trott force-pushed the optimize-vm branch from f0e3638 to ffdc082 Compare January 15, 2017 16:49

Trott added 3 commits January 15, 2017 08:49

benchmark: add benchmark for vm.runIn*()

95e90a0

Introduce benchmarks for vm.runInContext() and vm.runInThisContext().

vm: improve performance of vm.runIn*()

ffdc082

Optimize for common cases in vm.runInContext() and vm.runInThisContext().

squash

3967c08

jasnell approved these changes Jan 15, 2017

View reviewed changes

addaleax approved these changes Jan 15, 2017

View reviewed changes

bnoordhuis approved these changes Jan 16, 2017

View reviewed changes

mscdex added the performance Issues and PRs related to the performance of Node.js. label Jan 17, 2017

jasnell closed this Jan 18, 2017

italoacasas mentioned this pull request Jan 25, 2017

Proposal to update v7.x-staging #10989

Closed

italoacasas mentioned this pull request Jan 29, 2017

v7.5.0 proposal #11062

Merged

MylesBorins added lts-watch-v4.x labels Mar 8, 2017

MylesBorins removed the lts-watch-v4.x label May 8, 2017

MylesBorins added the backport-requested-v6.x label May 15, 2017

Trott removed the backport-requested-v6.x label May 15, 2017

This was referenced Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

vm: name anonymous functions #9388

Closed

gibfahn added the baking-for-lts PRs that need to wait before landing in a LTS release. label Jun 17, 2017

MylesBorins removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Aug 17, 2018

Trott deleted the optimize-vm branch January 13, 2022 22:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

vm: improve performance of vm.runIn*() #10816

vm: improve performance of vm.runIn*() #10816

Trott commented Jan 15, 2017

mscdex Jan 15, 2017

Trott commented Jan 15, 2017

mscdex commented Jan 15, 2017

Trott commented Jan 15, 2017

mscdex commented Jan 16, 2017

bnoordhuis left a comment

bnoordhuis Jan 16, 2017

Trott Jan 17, 2017

bnoordhuis Jan 16, 2017

mscdex Jan 16, 2017

Trott commented Jan 17, 2017

jasnell commented Jan 18, 2017

MylesBorins commented Mar 8, 2017

MylesBorins commented May 8, 2017

Trott commented May 8, 2017

MylesBorins commented May 15, 2017

Trott commented May 15, 2017

Trott commented May 15, 2017

MylesBorins commented May 16, 2017

gibfahn commented Jun 17, 2017

vm: improve performance of vm.runIn*() #10816

vm: improve performance of vm.runIn*() #10816

Conversation

Trott commented Jan 15, 2017

Checklist

Affected core subsystem(s)

mscdex Jan 15, 2017

Choose a reason for hiding this comment

Trott commented Jan 15, 2017

mscdex commented Jan 15, 2017

Trott commented Jan 15, 2017

mscdex commented Jan 16, 2017

bnoordhuis left a comment

Choose a reason for hiding this comment

bnoordhuis Jan 16, 2017

Choose a reason for hiding this comment

Trott Jan 17, 2017

Choose a reason for hiding this comment

bnoordhuis Jan 16, 2017

Choose a reason for hiding this comment

mscdex Jan 16, 2017

Choose a reason for hiding this comment

Trott commented Jan 17, 2017

jasnell commented Jan 18, 2017

MylesBorins commented Mar 8, 2017

MylesBorins commented May 8, 2017

Trott commented May 8, 2017

MylesBorins commented May 15, 2017

Trott commented May 15, 2017

Trott commented May 15, 2017

MylesBorins commented May 16, 2017

gibfahn commented Jun 17, 2017