Skip to content
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.

Sign up for GitHub

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

Jump to bottom

readline: remove the caching variable #14275

Closed
wants to merge 1 commit into from
Closed

readline: remove the caching variable #14275

wants to merge 1 commit into from

Conversation

robin98sun
Copy link
Contributor

line 486 and 525 contain for loops where a length property
is cached in a variable (for example, itemLen). This used
to be a performance optimization, but current V8 handles
the optimization internally.

these caching variables are removed, and using the length
property directly instead.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

readline: remove the caching variable
3d382f7 
line 486 and 525 contain for loops where a length property
is cached in a variable (for example, itemLen). This used
to be a performance optimization, but current V8 handles
the optimization internally.

these caching variables are removed, and using the length
property directly instead.
@nodejs-github-bot nodejs-github-bot added the readline Issues and PRs related to the built-in readline module. label Jul 16, 2017
Trott
Trott approved these changes Jul 16, 2017
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM if CI is green

@Trott Trott added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Jul 16, 2017
@TimothyGu TimothyGu added code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. and removed code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. labels Jul 16, 2017
@gibfahn gibfahn added the performance Issues and PRs related to the performance of Node.js. label Jul 16, 2017
@gibfahn
Copy link
Member

gibfahn commented Jul 16, 2017

cc/ @nodejs/performance

@Trott do you have a reference for this? Otherwise I guess we might need a benchmark run.

@benjamingr
Copy link
Member

These changes are not supposed to have any performance impact whatsoever - if anything - caching the length manually slows loops down. Changes LGTM

@benjamingr
Copy link
Member

@gibfahn reference - if you'd like a reference to avoid a benchmark run -
here's one of the better ones in terms of readability http://mrale.ph/blog/2014/12/24/array-length-caching.html

@gibfahn
Copy link
Member

gibfahn commented Jul 16, 2017

reference - if you'd like a reference to avoid a benchmark run

Seems pretty authoritative, thanks!

@tniessen tniessen self-assigned this Jul 16, 2017
@vsemozhetbyt
Copy link
Contributor

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

@refack
Copy link
Contributor

refack commented Jul 16, 2017 •
edited
Loading

Anyone know why this wasn't lint? compLen was undefined? I didn't see the , Now I'm 3 X Happy it's gone.

@Trott
Copy link
Member

Trott commented Jul 16, 2017

CI was green, but I canceled some of the slower ARM hosts and one of the two FreeBSD hosts in order to help with our CI backlog problem. IMO, the CI for this can be treated as green.

@robin98sun robin98sun changed the title [JSConf CN Code&Learn] readline: remove the caching variable readline: remove the caching variable Jul 19, 2017
@tniessen
Copy link
Member

Landed in 97c4033, thank you for your first contribution! 🎉

@tniessen tniessen closed this Jul 21, 2017
tniessen pushed a commit that referenced this pull request Jul 21, 2017
@tniessen
readline: remove the caching variable
97c4033 
Line 486 and 525 contain for loops where a length property is cached in
a variable (for example, itemLen). This used to be a performance
optimization, but current V8 handles the optimization internally.

These caching variables are removed, and the length property is used
directly instead.

PR-URL: #14275
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
addaleax pushed a commit that referenced this pull request Jul 22, 2017
@addaleax
readline: remove the caching variable
7aa52ff 
Line 486 and 525 contain for loops where a length property is cached in
a variable (for example, itemLen). This used to be a performance
optimization, but current V8 handles the optimization internally.

These caching variables are removed, and the length property is used
directly instead.

PR-URL: #14275
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Jul 24, 2017
@Fishrock123
readline: remove the caching variable
e54f75b 
Line 486 and 525 contain for loops where a length property is cached in
a variable (for example, itemLen). This used to be a performance
optimization, but current V8 handles the optimization internally.

These caching variables are removed, and the length property is used
directly instead.

PR-URL: #14275
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
@addaleax addaleax mentioned this pull request Jul 24, 2017
Closed
@addaleax addaleax mentioned this pull request Aug 2, 2017
Merged
@MylesBorins
Copy link
Contributor

holding off on this optimization for v6.x, lmk if we should reconsider

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

@gibfahn gibfahn gibfahn approved these changes

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

@tniessen tniessen tniessen approved these changes

@cjihrig cjihrig cjihrig approved these changes

@TimothyGu TimothyGu TimothyGu approved these changes

@benjamingr benjamingr benjamingr approved these changes

@Trott Trott Trott approved these changes

@jasnell jasnell jasnell approved these changes

@refack refack refack approved these changes

Assignees

@tniessen tniessen

Labels
code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. performance Issues and PRs related to the performance of Node.js. readline Issues and PRs related to the built-in readline module.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.