vm: refactor vm module by jasnell · Pull Request #11392 · nodejs/node · GitHub

Switch to the more efficient module.exports = {} pattern.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

vm

Somewhat related: perhaps we should move module.exports in each file to the bottom of the file for consistency and easier locatability?

LGTM and +1 for moving module.exports to the bottom of the file.

ok, moved module.exports to the end!

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

Standard location for module.exports = { is at top of file, not bottom, grep 'exports = {' lib/**/*.js to see this.

For good reason, IMO, when looking at a module, what it exports is among the most important information.

@sam-github IMHO having it at the bottom avoids any potential issues that could arise if exporting values assigned to variables for example.

I wouldn't call it "standard" by any stretch. Our style is a bit all over the map. Sometimes we use the exports.whatever = approach, other times we use module.exports = {} at the top, other places it's in the middle somewhere. Part of this is intended to put a stake in the ground with a consistent style and another part is to use the module.exports = {} pattern consistently which is more efficient on load up time and, as I understand it correctly, allows V8 to optimize more efficiently.

ok not "standard", but "majority"? I personally quite dislike the "exports at bottom" style, what a module exports is the most important thing about it, it should be at the top, and js has good support for this because of its scoping rules, but I'll live with any consistent style. Given how common exports.fu = ... is in the js world, I'd hope v8 would not be applying perf penalties to code that does that.

My bigger concern is the code churn and how it affects backportability, but if we can backport the refactors, I guess we'll be OK.

@sam-github It can incur a performance penalty due to the addition of hidden classes every time a new property is added, but hidden classes are still faster than dictionary mode. There is a way to force an object back into "fast" mode IIRC, but it's hacky and it's better to just define the properties in a literal from the get-go.

Switch to the more efficient module.exports = {} pattern.

Updated to resolve some CI failures...

@sam-github, here are the results of three benchmark runs after the refactor comparing to 7.5.0. At the results show, there is a modest but real performance improvement.

james@ubuntu:~/node/node$ ./node benchmark/compare.js --old node --new ./node vm > ~/vmbench
[00:01:08|% 100| 2/2 files | 60/60 runs | 4/4 configs]: Done
james@ubuntu:~/node/node$ cat ~/vmbench | Rscript benchmark/compare.R
                                                                    improvement confidence    p.value
 vm/run-in-context.js withSigintListener=0 breakOnSigint=0 n=1          11.62 %            0.08304273
 vm/run-in-context.js withSigintListener=0 breakOnSigint=1 n=1          11.60 %            0.11359883
 vm/run-in-context.js withSigintListener=1 breakOnSigint=0 n=1           2.41 %            0.73765864
 vm/run-in-context.js withSigintListener=1 breakOnSigint=1 n=1          -6.96 %            0.29611889
 vm/run-in-this-context.js withSigintListener=0 breakOnSigint=0 n=1      9.06 %            0.25190565
 vm/run-in-this-context.js withSigintListener=0 breakOnSigint=1 n=1      0.86 %            0.91037805
 vm/run-in-this-context.js withSigintListener=1 breakOnSigint=0 n=1     15.10 %            0.05376572
 vm/run-in-this-context.js withSigintListener=1 breakOnSigint=1 n=1      4.57 %            0.51519663
james@ubuntu:~/node/node$ ./node benchmark/compare.js --old node --new ./node vm > ~/vmbench
[00:01:08|% 100| 2/2 files | 60/60 runs | 4/4 configs]: Done
james@ubuntu:~/node/node$ cat ~/vmbench | Rscript benchmark/compare.R
                                                                    improvement confidence      p.value
 vm/run-in-context.js withSigintListener=0 breakOnSigint=0 n=1          25.82 %        *** 0.0007840776
 vm/run-in-context.js withSigintListener=0 breakOnSigint=1 n=1          -6.05 %            0.2196347443
 vm/run-in-context.js withSigintListener=1 breakOnSigint=0 n=1           6.36 %            0.4161236586
 vm/run-in-context.js withSigintListener=1 breakOnSigint=1 n=1           3.09 %            0.6493418279
 vm/run-in-this-context.js withSigintListener=0 breakOnSigint=0 n=1      7.04 %            0.3946533283
 vm/run-in-this-context.js withSigintListener=0 breakOnSigint=1 n=1      6.09 %            0.4692360799
 vm/run-in-this-context.js withSigintListener=1 breakOnSigint=0 n=1      3.76 %            0.6699398702
 vm/run-in-this-context.js withSigintListener=1 breakOnSigint=1 n=1     10.32 %            0.2190428802
james@ubuntu:~/node/node$ rm ~/vmbench
james@ubuntu:~/node/node$ ./node benchmark/compare.js --old node --new ./node vm > ~/vmbench
[00:00:49|% 100| 2/2 files | 60/60 runs | 4/4 configs]: Done
james@ubuntu:~/node/node$ cat ~/vmbench | Rscript benchmark/compare.R
                                                                    improvement confidence    p.value
 vm/run-in-context.js withSigintListener=0 breakOnSigint=0 n=1           8.48 %            0.05767758
 vm/run-in-context.js withSigintListener=0 breakOnSigint=1 n=1          10.78 %          * 0.01045460
 vm/run-in-context.js withSigintListener=1 breakOnSigint=0 n=1           5.10 %            0.34595699
 vm/run-in-context.js withSigintListener=1 breakOnSigint=1 n=1          11.08 %            0.05057880
 vm/run-in-this-context.js withSigintListener=0 breakOnSigint=0 n=1      3.14 %            0.60463224
 vm/run-in-this-context.js withSigintListener=0 breakOnSigint=1 n=1      1.35 %            0.81451854
 vm/run-in-this-context.js withSigintListener=1 breakOnSigint=0 n=1     15.06 %          * 0.01239632
 vm/run-in-this-context.js withSigintListener=1 breakOnSigint=1 n=1     -6.01 %            0.18145218

Marking as semver-major because this does cause changes to the stack trace output

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

Is the stack trace part of the API? I thought it was just the first line, the error message? If it can't be backported, vm maintainers will have some pain, but I'll let them speak for themselves on that.

I’m pretty sure we have (I have?) treated change that only affect the content of stack traces as semver-patch in the past.

If it doesn't have to be semver-major then ++ :-)

Pulling the semver-major label back off.

Switch to the more efficient module.exports = {} pattern. PR-URL: #11392 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Landed in 804bb22

Switch to the more efficient module.exports = {} pattern. PR-URL: #11392 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

needs a backport to land on v4

Switch to the more efficient module.exports = {} pattern. PR-URL: #11392 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

nodejs-github-bot added the vm Issues and PRs related to the vm subsystem. label Feb 15, 2017

targos approved these changes Feb 15, 2017

View reviewed changes

jasnell force-pushed the vm-refactor branch from f256211 to ece61bd Compare February 15, 2017 21:26

cjihrig approved these changes Feb 16, 2017

View reviewed changes

vm: refactor vm module

0de2914

Switch to the more efficient module.exports = {} pattern.

jasnell force-pushed the vm-refactor branch from ece61bd to 0de2914 Compare February 16, 2017 17:19

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 16, 2017

jasnell removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 17, 2017

jasnell added a commit that referenced this pull request Feb 17, 2017

vm: refactor vm module

804bb22

Switch to the more efficient module.exports = {} pattern. PR-URL: #11392 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

jasnell closed this Feb 17, 2017

addaleax pushed a commit that referenced this pull request Feb 22, 2017

vm: refactor vm module

f55c628

Switch to the more efficient module.exports = {} pattern. PR-URL: #11392 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

italoacasas mentioned this pull request Feb 25, 2017

7.7.0 Proposal #11553

Merged

jasnell added the dont-land-on-v4.x label Mar 7, 2017

jasnell added a commit that referenced this pull request Mar 7, 2017

vm: refactor vm module

f3546b7

Switch to the more efficient module.exports = {} pattern. PR-URL: #11392 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

MylesBorins pushed a commit that referenced this pull request Mar 9, 2017

vm: refactor vm module

a55af77

Switch to the more efficient module.exports = {} pattern. PR-URL: #11392 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

MylesBorins mentioned this pull request Mar 9, 2017

v6.10.1 proposal #11759

Merged

vm: refactor vm module by jasnell · Pull Request #11392 · nodejs/node · GitHub

Uh oh!

Uh oh!

vm: refactor vm module - #11392

vm: refactor vm module#11392jasnell wants to merge 1 commit intonodejs:masternodejs/node:masterfrom jasnell:vm-refactorjasnell/node:vm-refactorCopy head branch name to clipboard

Conversation

jasnell commented Feb 15, 2017

Checklist

Affected core subsystem(s)

Uh oh!

mscdex commented Feb 15, 2017

Uh oh!

targos left a comment

Choose a reason for hiding this comment

Uh oh!

jasnell commented Feb 15, 2017

Uh oh!

jasnell commented Feb 15, 2017

Uh oh!

sam-github commented Feb 16, 2017

Uh oh!

mscdex commented Feb 16, 2017

Uh oh!

jasnell commented Feb 16, 2017

Uh oh!

sam-github commented Feb 16, 2017

Uh oh!

mscdex commented Feb 16, 2017

Uh oh!

jasnell commented Feb 16, 2017

Uh oh!

jasnell commented Feb 16, 2017

Uh oh!

jasnell commented Feb 16, 2017

Uh oh!

sam-github commented Feb 16, 2017

Uh oh!

addaleax commented Feb 16, 2017

Uh oh!

jasnell commented Feb 16, 2017

Uh oh!

jasnell commented Feb 17, 2017

Uh oh!

jasnell commented Feb 17, 2017

Uh oh!

jasnell commented Mar 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

vm: refactor vm module#11392
jasnell wants to merge 1 commit into
nodejs:masterfrom
jasnell:vm-refactor