meta: remove let from for loops by MylesBorins · Pull Request #8873 · nodejs/node · GitHub

Checklist

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

Affected core subsystem(s)

lib

Description of change

This is a known de-opt. It may not be 100% necessary in all cases but it
seems like a decent enough idea to avoid it.

If individuals are interested we can create a linting rule for this

LGTM if CI is ok with it: https://ci.nodejs.org/job/node-test-pull-request/4343/

Looks like there's a variable name conflict between a var and a const.

fixed

new ci: https://ci.nodejs.org/job/node-test-pull-request/4344/

Does this have any effect on the benchmarks in master?

windows failure was a timeout
https://ci.nodejs.org/job/node-test-binary-windows/4129/RUN_SUBSET=0,VS_VERSION=vs2015,label=win2012r2/tapTestReport/test.tap-238/

likely infra related

Still LGTM.

LGTM

would someone from @nodejs/benchmarking help me run some tests to see if this actually makes any difference?

LGTM

would someone from @nodejs/benchmarking help me run some tests to see if this actually makes any difference?

I doubt it makes a practical difference for some of these changes. For example calling stream.unpipe() is quite rare. So I just benchmarked the punycode changes:

# compile master 
git checkout master
./configure
make -j8
cp ./node ./node-master

# compile pr 8873
apply-pr 8873
make -j8
cp ./node ./node-pr8873

# benchmark
node benchmark/compare.js
  --old ./node-master --new ./node-pr8873
  --filter punycode -- net | tee punycode-benchmark.csv
cat punycode-benchmark.csv | Rscript ./benchmark/compare.R

                                                          improvement significant      p.value
val="belgië.icom.museum" n=1024 method="icu"                  -0.43 %             2.903102e-01
val="belgië.icom.museum" n=1024 method="punycode"              5.02 %         *** 5.920579e-12
val="českárepublika.icom.museum" n=1024 method="icu"          -0.08 %             8.350579e-01
val="českárepublika.icom.museum" n=1024 method="punycode"      7.01 %         *** 1.656402e-05
val="éire.icom.museum" n=1024 method="icu"                    -0.30 %             6.925716e-01
val="éire.icom.museum" n=1024 method="punycode"                5.72 %         *** 1.383591e-06
val="ísland.icom.museum" n=1024 method="icu"                   1.48 %             1.855450e-01
val="ísland.icom.museum" n=1024 method="punycode"              6.12 %         *** 3.057111e-07
val="magyarország.icom.museum" n=1024 method="icu"            -1.11 %             2.709653e-01
val="magyarország.icom.museum" n=1024 method="punycode"        6.48 %         *** 2.493271e-05
val="österreich.icom.museum" n=1024 method="icu"              -1.36 %             3.925142e-01
val="österreich.icom.museum" n=1024 method="punycode"          6.79 %         *** 1.343576e-05
val="ελλάδα.icom.museum" n=1024 method="icu"                   0.48 %             7.566853e-01
val="ελλάδα.icom.museum" n=1024 method="punycode"              5.80 %         *** 5.955543e-04
val="κυπρος.icom.museum" n=1024 method="icu"                  -1.55 %             3.175751e-01
val="κυπρος.icom.museum" n=1024 method="punycode"              4.83 %          ** 3.246308e-03
val="беларусь.icom.museum" n=1024 method="icu"                -0.23 %             7.148822e-01
val="беларусь.icom.museum" n=1024 method="punycode"            8.42 %         *** 6.554571e-04
val="българия.icom.museum" n=1024 method="icu"                 3.27 %             1.822171e-01
val="българия.icom.museum" n=1024 method="punycode"           10.95 %         *** 3.395971e-06
val="איקו״ם.ישראל.museum" n=1024 method="icu"                 -2.01 %             3.947640e-01
val="איקו״ם.ישראל.museum" n=1024 method="punycode"             5.00 %          ** 1.350123e-03
val="افغانستا.icom.museum" n=1024 method="icu"                -0.13 %             7.612569e-01
val="افغانستا.icom.museum" n=1024 method="punycode"            3.29 %           * 3.289352e-02
val="الأردن.icom.museum" n=1024 method="icu"                  -2.24 %             2.582112e-01
val="الأردن.icom.museum" n=1024 method="punycode"              4.67 %           * 1.154801e-02
val="الجزائر.icom.museum" n=1024 method="icu"                 -0.02 %             9.596995e-01
val="الجزائر.icom.museum" n=1024 method="punycode"             7.99 %         *** 2.736761e-04
val="القمر.icom.museum" n=1024 method="icu"                    0.47 %             2.381307e-01
val="القمر.icom.museum" n=1024 method="punycode"               5.69 %         *** 2.288273e-06
val="ايران.icom.museum" n=1024 method="icu"                   -0.33 %             8.802176e-01
val="ايران.icom.museum" n=1024 method="punycode"               2.77 %           * 1.163676e-02
val="تشادر.icom.museum" n=1024 method="icu"                   -0.11 %             7.246581e-01
val="تشادر.icom.museum" n=1024 method="punycode"               8.00 %         *** 3.214189e-05
val="مصر.icom.museum" n=1024 method="icu"                     -1.09 %             2.214773e-01
val="مصر.icom.museum" n=1024 method="punycode"                 6.64 %         *** 1.282843e-04
val="भारत.icom.museum" n=1024 method="icu"                    -0.46 %             3.967156e-01
val="भारत.icom.museum" n=1024 method="punycode"                5.03 %          ** 1.413984e-03
val="বাংলাদেশ.icom.museum" n=1024 method="icu"                -1.03 %             4.592365e-01
val="বাংলাদেশ.icom.museum" n=1024 method="punycode"            5.40 %           * 1.164211e-02
val="中国.icom.museum" n=1024 method="icu"                     3.20 %             5.088870e-02
val="中国.icom.museum" n=1024 method="punycode"                7.80 %         *** 2.616430e-06
val="日本.icom.museum" n=1024 method="icu"                     1.02 %             1.124426e-01
val="日本.icom.museum" n=1024 method="punycode"                4.01 %         *** 6.611998e-06

As you can see, the changes makes about a 5% statistically significant improvement in all punycode cases. If I didn't want the icu results I could have added --set method=punycode to compare.js.

PS: I'm not a @nodejs/benchmarking member, they may have a more historic (not just master) perspective.

This is a known de-opt. It may not be 100% necessary in all cases but it seems like a decent enough idea to avoid it. PR-URL: nodejs#8873 Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com> Reviewed-By: Johan Bergström <bugs@bergstroem.nu> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

@thealphanerd Looks like this has caused a couple of linter failures:

$ make lint              
./node tools/eslint/bin/eslint.js --cache --rulesdir=tools/eslint-rules \
      benchmark lib test tools

/Users/gib/dev/node/lib/_stream_readable.js
  664:30  error  'i' is constant         no-const-assign
  670:9   error  'i' is already defined  no-redeclare

✖ 2 problems (2 errors, 0 warnings)

make: *** [jslint] Error 1

@gibfahn ah noodles. PR incoming

Also for future reference (I missed this), lib: would have been a better subsystem name instead of meta:.

I'm planning to force push to fix the problem. I can change the label when doing so

This is a known de-opt. It may not be 100% necessary in all cases but it seems like a decent enough idea to avoid it. PR-URL: nodejs#8873 Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com> Reviewed-By: Johan Bergström <bugs@bergstroem.nu> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

force pushed with 2e568d9 to fix linting problem. Sorry about that, not sure how it regressed

This is a known de-opt. It may not be 100% necessary in all cases but it seems like a decent enough idea to avoid it. PR-URL: #8873 Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com> Reviewed-By: Johan Bergström <bugs@bergstroem.nu> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

This adds a new ESLint tool to check for let declarations within the for, forIn, forOf expressions. Fixes: nodejs#9045 Ref: nodejs#8873

This adds a new ESLint tool to check for let declarations within the for, forIn, forOf expressions. Fixes: nodejs#9045 Ref: nodejs#8873 PR-URL: nodejs#9049 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Teddy Katz <teddy.katz@gmail.com> Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

This adds a new ESLint tool to check for let declarations within the for, forIn, forOf expressions. Fixes: #9045 Ref: #8873 PR-URL: #9049 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Teddy Katz <teddy.katz@gmail.com> Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Just leaving the note here that if this lands in v4, it needs to do so together with #9171.

This adds a new ESLint tool to check for let declarations within the for, forIn, forOf expressions. Fixes: #9045 Ref: #8873 PR-URL: #9049 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Teddy Katz <teddy.katz@gmail.com> Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

This is a known de-opt. It may not be 100% necessary in all cases but it seems like a decent enough idea to avoid it. Ref: nodejs#9553 PR-URL: nodejs#8873 Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com> Reviewed-By: Johan Bergström <bugs@bergstroem.nu> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

This adds a new ESLint tool to check for let declarations within the for, forIn, forOf expressions. Fixes: nodejs#9045 Ref: nodejs#9553 Ref: nodejs#8873 PR-URL: nodejs#9049 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Teddy Katz <teddy.katz@gmail.com> Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

This is a known de-opt. It may not be 100% necessary in all cases but it seems like a decent enough idea to avoid it. Ref: #9553 PR-URL: #8873 Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com> Reviewed-By: Johan Bergström <bugs@bergstroem.nu> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

This adds a new ESLint tool to check for let declarations within the for, forIn, forOf expressions. Fixes: #9045 Ref: #9553 Ref: #8873 PR-URL: #9049 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Teddy Katz <teddy.katz@gmail.com> Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

This just landed upstream in V8 5.7: https://bugs.chromium.org/p/v8/issues/detail?id=5666#c10. Starting in Node.js v8.x, we will no longer need to avoid let/const. \o/

@Trott we should remove the lint rule that we have in V8... what is the best way to stage something like this until then

@thealphanerd Not sure. Maybe open a PR and label it with all the appropriate dont-land-on labels?

The above suggestion runs the risk of PRs landing on master that won't lint in LTS, but I don't think that will be too much of a problem in practice. We don't get a lot of let-in-loops PRs for the lib dir and when we do, reviewers usually call them out.

Even though const/let will soon be handled by TurboFan, how does that currently stack up against just using var (which I assume still "goes through" Crankshaft)?

@ofrobots ?

Unless there is another reason to go through TF (e.g. try-catch, other ES6 features), the default would still be Crankshaft.

However, by the time this ships with Node.js 8.x, the default pipeline used by V8 would be quite different with Ignition and TurboFan being significantly more prominent. Exact details aren't locked down just yet.

I personally don't expect var, let and const to perform any different – but these don't exist in a vacuum. These are used in the context of other code and if the optimizer used to compile this surrounding code is changed, then you're comparing the capabilities of TF and CS on the surrounding code, not just var and let.

Try it out and report back if see unexpected performance results.

MylesBorins added the meta Issues and PRs related to the general management of the project. label Sep 30, 2016

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Sep 30, 2016

MylesBorins changed the title meta: remove let for from loops meta: remove let from for loops Sep 30, 2016

MylesBorins force-pushed the for-de-let branch from dd23ad3 to abdf1a0 Compare September 30, 2016 22:34

MylesBorins force-pushed the for-de-let branch from abdf1a0 to 42b97cf Compare October 1, 2016 00:41

targos approved these changes Oct 1, 2016

View reviewed changes

lpinca approved these changes Oct 1, 2016

View reviewed changes

jbergstroem approved these changes Oct 1, 2016

View reviewed changes

imyller approved these changes Oct 1, 2016

View reviewed changes

AndreasMadsen mentioned this pull request Oct 1, 2016

Prototype for running a subset of benchmarks on a pull request nodejs/benchmarking#58

Merged

MylesBorins force-pushed the for-de-let branch from 42b97cf to 95ba482 Compare October 4, 2016 15:33

MylesBorins merged commit 95ba482 into nodejs:master Oct 4, 2016

MylesBorins added the lts-watch-v4.x label Oct 4, 2016

gibfahn mentioned this pull request Oct 4, 2016

doc: correct grammar in README.md #8892

Closed

2 tasks

mscdex removed the meta Issues and PRs related to the general management of the project. label Oct 4, 2016

MylesBorins mentioned this pull request Oct 12, 2016

Create Lint Rule to avoid let in for loops #9045

Closed

jalafel mentioned this pull request Oct 12, 2016

tools: avoid let in for loops #9049

Closed

2 tasks

jalafel added a commit to jalafel/node that referenced this pull request Oct 14, 2016

tools: avoid let in for loops

8b704d6

This adds a new ESLint tool to check for let declarations within the for, forIn, forOf expressions. Fixes: nodejs#9045 Ref: nodejs#8873

MylesBorins mentioned this pull request Nov 11, 2016

Backport removing let in for loop declaration #9553

Closed

3 tasks

MylesBorins added dont-land-on-v4.x and removed lts-watch-v4.x labels Nov 18, 2016

MylesBorins mentioned this pull request Nov 22, 2016

v4.7.0 proposal #9736

Merged

cjihrig mentioned this pull request Dec 2, 2016

let var in loop (question) hapijs/hapi#3403

Closed

MylesBorins deleted the for-de-let branch November 14, 2017 17:43

meta: remove let from for loops by MylesBorins · Pull Request #8873 · nodejs/node · GitHub

Uh oh!

Uh oh!

meta: remove let from for loops - #8873

meta: remove let from for loops#8873MylesBorins merged 1 commit intonodejs:masternodejs/node:masterfrom MylesBorins:for-de-letMylesBorins/node:for-de-letCopy head branch name to clipboard

Conversation

MylesBorins commented Sep 30, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

mscdex commented Oct 1, 2016

Uh oh!

mscdex commented Oct 1, 2016

Uh oh!

MylesBorins commented Oct 1, 2016

Uh oh!

ChALkeR commented Oct 1, 2016

Uh oh!

MylesBorins commented Oct 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mscdex commented Oct 1, 2016

Uh oh!

lpinca left a comment

Choose a reason for hiding this comment

Uh oh!

jbergstroem left a comment

Choose a reason for hiding this comment

Uh oh!

MylesBorins commented Oct 1, 2016

Uh oh!

imyller left a comment

Choose a reason for hiding this comment

Uh oh!

AndreasMadsen commented Oct 2, 2016

Uh oh!

gibfahn commented Oct 4, 2016

Uh oh!

MylesBorins commented Oct 4, 2016

Uh oh!

mscdex commented Oct 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MylesBorins commented Oct 4, 2016

Uh oh!

MylesBorins commented Oct 4, 2016

Uh oh!

addaleax commented Oct 19, 2016

Uh oh!

ofrobots commented Dec 2, 2016

Uh oh!

MylesBorins commented Dec 2, 2016

Uh oh!

Trott commented Dec 2, 2016

Uh oh!

Trott commented Dec 2, 2016

Uh oh!

mscdex commented Dec 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ofrobots commented Dec 3, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

meta: remove let from for loops#8873
MylesBorins merged 1 commit into
nodejs:masterfrom
MylesBorins:for-de-let

MylesBorins commented Oct 1, 2016 •
edited
Loading

mscdex commented Oct 4, 2016 •
edited
Loading

mscdex commented Dec 2, 2016 •
edited
Loading