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

meta: remove let from for loops #8873

Merged
merged 1 commit into from Oct 4, 2016

Conversation

Projects
None yet
@MylesBorins
Member

MylesBorins commented Sep 30, 2016

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

@MylesBorins MylesBorins added the meta label Sep 30, 2016

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

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex
Contributor

mscdex commented Oct 1, 2016

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Oct 1, 2016

Contributor

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

Contributor

mscdex commented Oct 1, 2016

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

@MylesBorins

This comment has been minimized.

Show comment
Hide comment
Member

MylesBorins commented Oct 1, 2016

@targos

targos approved these changes Oct 1, 2016

@ChALkeR

This comment has been minimized.

Show comment
Hide comment
@ChALkeR

ChALkeR Oct 1, 2016

Member

Does this have any effect on the benchmarks in master?

Member

ChALkeR commented Oct 1, 2016

Does this have any effect on the benchmarks in master?

@MylesBorins

This comment has been minimized.

Show comment
Hide comment
Member

MylesBorins commented Oct 1, 2016

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Oct 1, 2016

Contributor

Still LGTM.

Contributor

mscdex commented Oct 1, 2016

Still LGTM.

@lpinca

lpinca approved these changes Oct 1, 2016

LGTM

@jbergstroem

LGTM

@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Oct 1, 2016

Member

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

Member

MylesBorins commented Oct 1, 2016

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

@imyller

imyller approved these changes Oct 1, 2016

LGTM

@AndreasMadsen

This comment has been minimized.

Show comment
Hide comment
@AndreasMadsen

AndreasMadsen Oct 2, 2016

Member

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.

Member

AndreasMadsen commented Oct 2, 2016

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.

meta: remove let from for loops
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>

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

@gibfahn gibfahn referenced this pull request Oct 4, 2016

Closed

doc: correct grammar in README.md #8892

2 of 2 tasks complete
@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Oct 4, 2016

Member

@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
Member

gibfahn commented Oct 4, 2016

@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
@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Oct 4, 2016

Member

@gibfahn ah noodles. PR incoming

Member

MylesBorins commented Oct 4, 2016

@gibfahn ah noodles. PR incoming

@mscdex mscdex removed the meta label Oct 4, 2016

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Oct 4, 2016

Contributor

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

Contributor

mscdex commented Oct 4, 2016

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

@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Oct 4, 2016

Member

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

Member

MylesBorins commented Oct 4, 2016

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

MylesBorins added a commit to MylesBorins/node that referenced this pull request Oct 4, 2016

lib: remove let from for loops
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>
@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Oct 4, 2016

Member

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

Member

MylesBorins commented Oct 4, 2016

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

jasnell added a commit that referenced this pull request Oct 6, 2016

lib: remove let from for loops
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>

Fishrock123 added a commit that referenced this pull request Oct 11, 2016

lib: remove let from for loops
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>

@jessqt jessqt referenced this pull request Oct 12, 2016

Closed

tools: avoid let in for loops #9049

1 of 2 tasks complete

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

tools: avoid let in for loops
This adds a new ESLint tool to check for let
declarations within the for, forIn, forOf expressions.

Fixes: nodejs#9045
Ref: nodejs#8873

Trott added a commit to Trott/io.js that referenced this pull request Oct 14, 2016

tools: avoid let in for loops
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>

jasnell added a commit that referenced this pull request Oct 17, 2016

tools: avoid let in for loops
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>
@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Oct 19, 2016

Member

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

Member

addaleax commented Oct 19, 2016

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

MylesBorins added a commit that referenced this pull request Nov 11, 2016

tools: avoid let in for loops
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>

@MylesBorins MylesBorins referenced this pull request Nov 11, 2016

Closed

Backport removing let in for loop declaration #9553

3 of 3 tasks complete

MylesBorins added a commit to MylesBorins/node that referenced this pull request Nov 11, 2016

lib: remove let from for loops
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>

MylesBorins added a commit to MylesBorins/node that referenced this pull request Nov 11, 2016

tools: avoid let in for loops
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>

MylesBorins added a commit that referenced this pull request Nov 14, 2016

lib: remove let from for loops
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>

MylesBorins added a commit that referenced this pull request Nov 14, 2016

tools: avoid let in for loops
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>

@MylesBorins MylesBorins referenced this pull request Nov 22, 2016

Merged

v4.7.0 proposal #9736

@ofrobots

This comment has been minimized.

Show comment
Hide comment
@ofrobots

ofrobots Dec 2, 2016

Contributor

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/

Contributor

ofrobots commented Dec 2, 2016

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/

@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Dec 2, 2016

Member

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

Member

MylesBorins commented Dec 2, 2016

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

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Dec 2, 2016

Member

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

Member

Trott commented Dec 2, 2016

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

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Dec 2, 2016

Member

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.

Member

Trott commented Dec 2, 2016

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.

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Dec 2, 2016

Contributor

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 ?

Contributor

mscdex commented Dec 2, 2016

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 ?

@cjihrig cjihrig referenced this pull request Dec 2, 2016

Closed

let var in loop (question) #3403

@ofrobots

This comment has been minimized.

Show comment
Hide comment
@ofrobots

ofrobots Dec 3, 2016

Contributor

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.

Contributor

ofrobots commented Dec 3, 2016

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 MylesBorins deleted the MylesBorins:for-de-let branch Nov 14, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment