tools: enable ESLint no-var rule in lib #42573
Conversation
Trott
added
the
commit-queue-rebase
|
Review requested: |
nodejs-github-bot
added
lib / src
lib/internal/async_hooks.js
Outdated
| // Using var here instead of let because "for (var ...)" is faster than let. | ||
| // Refs: https://github.com/nodejs/node/pull/30380#issuecomment-552948364 | ||
| for (var i = 0; i < active_hooks.array.length; i++) { | ||
| for (let i = 0; i < active_hooks.array.length; i++) { |
There was a problem hiding this comment.
I would restrain from making this change unless we have ample evidence that it's as fast as.
There was a problem hiding this comment.
@mcollina the for(var...) loop in os.networkInterfaces() was changed to for(let...) in #42563 and I ran a benchmark for that in #42563 (comment) and there was no significant perf regression. Could the results be different for these for loops?
There was a problem hiding this comment.
This is one of the hottest loops of the whole Node.js. Keeping a var here does not hurt anybody - most contributors should not even come to this file. Let this be and avoid future problems.
There was a problem hiding this comment.
Better to run a benchmark for this PR as well to find out if it's as fast or not. The performance edge cases (no matter the condition, hot or cold paths) were rather fixed in V8 years ago, as also seen in the mentioned PR (#42563).
There was a problem hiding this comment.
I'll use comments to disable linting for the ones @mcollina has flagged, and then we can do benchmarks on them or leave them as appropriate without holding up landing the rest of this.
lib/internal/async_hooks.js
Outdated
| // Using var here instead of let because "for (var ...)" is faster than let. | ||
| // Refs: https://github.com/nodejs/node/pull/30380#issuecomment-552948364 | ||
| for (var i = 0; i < active_hooks.array.length; i++) { | ||
| for (let i = 0; i < active_hooks.array.length; i++) { |
There was a problem hiding this comment.
I would restrain from making this change unless we have ample evidence that it's as fast as.
lib/internal/js_stream_socket.js
Outdated
| @@ -171,7 +171,7 @@ class JSStreamSocket extends Socket { | |||
|
|
|||
| this.stream.cork(); | |||
| // Use `var` over `let` for performance optimization. | |||
| for (var i = 0; i < bufs.length; ++i) | |||
| for (let i = 0; i < bufs.length; ++i) | |||
There was a problem hiding this comment.
I would restrain from making this change unless we have ample evidence that it's as fast as.
|
@mcollina I've reverted the three loops you flagged and instead disabled the lint rule for those lines. PTAL. Thanks! |
github-actions
bot
removed
the
request-ci
This comment was marked as outdated.
This comment was marked as outdated.
Trott
added
the
commit-queue
nodejs-github-bot
removed
the
commit-queue
|
Landed in aed18df...b07894d |
PR-URL: #42573 Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Akhil Marsonya <akhil.marsonya27@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
PR-URL: #42573 Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Akhil Marsonya <akhil.marsonya27@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
PR-URL: nodejs#42573 Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Akhil Marsonya <akhil.marsonya27@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
PR-URL: nodejs#42573 Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Akhil Marsonya <akhil.marsonya27@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
PR-URL: #42573 Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Akhil Marsonya <akhil.marsonya27@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
PR-URL: #42573 Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Akhil Marsonya <akhil.marsonya27@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
First commit is from #42563. Other two commits change the rest of the files in lib and enables the ESLint rule.