buffer: Reverting change in let to fix regressions in buffer operations by gareth-ellis · Pull Request #5819 · nodejs/node · GitHub

Pull Request check-list

Please make sure to review and check all of these items:

Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
this change (including linting)?
Is the commit message formatted according to CONTRIBUTING.md?
If this change fixes a bug (or a performance problem), is a regression
test (or a benchmark) included?
Is a documentation update included (if this change modifies
existing APIs, or introduces new ones)?

NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.

Affected core subsystem(s)

buffer

Description of change

Using let in for loops showed a regression in 4.4.0. @ofrobots suggested
that we avoid using let in for loops until TurboFan becomes the default
optimiser.

As discussed in nodejs/benchmarking#38

Just wondering why no var here. Looks like it was there previously 2ac47f8. At this point since its a jslint error all I can think of is that we have tightened up the lint rules ?

So this was to address a lint issue.

Really the var i in the for loop before should be declared outside of the if statement, so have added another commit to fix this,

lgtm, CI: https://ci.nodejs.org/job/node-test-pull-request/1988/

Can you include specific information about the regression in the commit message? I'm not sure if this is testable, but in the future when a developer wants to reintroduce let it will be helpful to clearly understand why this was reverted.

Also include a Ref: to the benchmarking issue referenced in the OP.

Code change LGTM.

LGTM (with @trevnorris' nits addressed)

Lgtm

nit: The two commits should be squash, and the commit message should be wrapped to 72 characters (currently the first lint in the comment is 73)

otherwise LGTM

Using let in for loops showed a regression in 4.4.0. @ofrobots suggested that we avoid using let in for loops until TurboFan becomes the default optimiser. The regression that was detected was when looking at how long it took to create a new buffer from an array of data. When using `for (let i=0; i<length; i++) ` we saw the operation take almost 40% longer compared to `var i=0`. Ref: http://github.com/nodejs/benchmarking/issues/38

Have squashed the commits and expanded commit message.

👍

excellent thanks

Isn't it let usage in general and not just loops? There's a let in .toString() that could easily change to var.

@mscdex perhaps we should open another issue regarding no longer using let in the code base... there are quite a few lets (including others in for loops).

The biggest push back I saw what that it is a micro optimization for non-hot code

Thoughts?

I just did a quick test, similar to the benchmark I ran originally, but this time looking at .toString

I got two builds, one with var result; and the second with let result;

first of all, with let
*let_tostring/Release/node to-string-test/to-string-test.js *
total time: 5.008s -- iterations: 199 -- ops/sec: 39.74 -- average time: 0.03s -- variance: 2.18%
total time: 5.018s -- iterations: 202 -- ops/sec: 40.26 -- average time: 0.02s -- variance: 1.47%
total time: 5.012s -- iterations: 201 -- ops/sec: 40.1 -- average time: 0.02s -- variance: 1.5%
total time: 5.016s -- iterations: 202 -- ops/sec: 40.27 -- average time: 0.02s -- variance: 1.51%
total time: 5.001s -- iterations: 202 -- ops/sec: 40.39 -- average time: 0.02s -- variance: 1.87%
total time: 5.012s -- iterations: 203 -- ops/sec: 40.5 -- average time: 0.02s -- variance: 1.88%
total time: 5.012s -- iterations: 203 -- ops/sec: 40.5 -- average time: 0.02s -- variance: 1.88%

*var_tostring/Release/node to-string-test/to-string-test.js *
total time: 5.024s -- iterations: 197 -- ops/sec: 39.21 -- average time: 0.03s -- variance: 2.26%
total time: 5.006s -- iterations: 200 -- ops/sec: 39.95 -- average time: 0.03s -- variance: 0.89%
total time: 5.016s -- iterations: 201 -- ops/sec: 40.07 -- average time: 0.02s -- variance: 1.22%
total time: 5s -- iterations: 201 -- ops/sec: 40.2 -- average time: 0.02s -- variance: 1.33%
total time: 5.007s -- iterations: 202 -- ops/sec: 40.34 -- average time: 0.02s -- variance: 1.8%
total time: 5.015s -- iterations: 203 -- ops/sec: 40.48 -- average time: 0.02s -- variance: 1.85%
total time: 5.015s -- iterations: 203 -- ops/sec: 40.48 -- average time: 0.02s -- variance: 1.89%

The amount the results are bouncing around, I'd suggest that the two are performing the same.

My benchmark, was measuring how many operations /second I could get when repeating this:

var TEXT = "Lorem Ipsum has been the industry's standard dummy text ever since the 1500s, when an unknown printer took a galley of type and scrambled it to make a type specimen book. It has survived not only five centuries, but also the leap into electronic typesetting, remaining essentially unchanged.";
var ITERATIONS = 200000;
var buffer = new Buffer(TEXT, 'utf-8');
var stringFromBuffer;

function test() {
        for(var i=0;i<ITERATIONS;i++) {
                stringFromBuffer = buffer.toString();
        }
}

harness.run_test(test);

LGTM, new CI: https://ci.nodejs.org/job/node-test-pull-request/2038

ok looks like we have a number of lgtm. @benjamingr you planning to land ? If not I can look at doing it later today

Sure, I'll land.

Using let in for loops showed a regression in 4.4.0. @ofrobots suggested that we avoid using let in for loops until TurboFan becomes the default optimiser. The regression that was detected was when looking at how long it took to create a new buffer from an array of data. When using `for (let i=0; i<length; i++) ` we saw the operation take almost 40% longer compared to `var i=0`. PR-URL: #5819 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Trevor Norris <trevnorris@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> Ref: http://github.com/nodejs/benchmarking/issues/38

Landed in 443c2d5 , thanks!

Using let in for loops showed a regression in 4.4.0. @ofrobots suggested that we avoid using let in for loops until TurboFan becomes the default optimiser. The regression that was detected was when looking at how long it took to create a new buffer from an array of data. When using `for (let i=0; i<length; i++) ` we saw the operation take almost 40% longer compared to `var i=0`. PR-URL: #5819 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Trevor Norris <trevnorris@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> Ref: http://github.com/nodejs/benchmarking/issues/38

@nodejs/lts we usually wait for things to spend time on v5 before backporting. This change is unlikely to cause breakage, and will result in immediate performance improvements. Are people opposed to having this land in this weeks release?

No objections

Using let in for loops showed a regression in 4.4.0. @ofrobots suggested that we avoid using let in for loops until TurboFan becomes the default optimiser. The regression that was detected was when looking at how long it took to create a new buffer from an array of data. When using `for (let i=0; i<length; i++) ` we saw the operation take almost 40% longer compared to `var i=0`. PR-URL: #5819 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Trevor Norris <trevnorris@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> Ref: http://github.com/nodejs/benchmarking/issues/38

gareth-ellis changed the title buffer: changing let in for loops back to var buffer: Reverting change in let to fix regressions in buffer operations Mar 21, 2016

mscdex added the buffer Issues and PRs related to the buffer subsystem. label Mar 21, 2016

mhdawson reviewed Mar 21, 2016
View reviewed changes

Fishrock123 added the v8 engine Issues and PRs related to the V8 dependency. label Mar 21, 2016

jasnell added the lts-watch-v4.x label Mar 21, 2016

mscdex mentioned this pull request Mar 21, 2016

let slows down performance by 5 times #5815

Closed

jasnell closed this Mar 23, 2016

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Mar 30, 2016

evanlucas mentioned this pull request Mar 31, 2016

v5.10.0 Release Proposal #5970

Closed

gareth-ellis mentioned this pull request Oct 11, 2016

feat(let-const-var): The test for let & const & var nodejs/benchmarking#63

Closed

buffer: Reverting change in let to fix regressions in buffer operations by gareth-ellis · Pull Request #5819 · nodejs/node · GitHub

Uh oh!

Uh oh!

buffer: Reverting change in let to fix regressions in buffer operations - #5819

buffer: Reverting change in let to fix regressions in buffer operations#5819gareth-ellis wants to merge 1 commit intonodejs:masternodejs/node:masterfrom gareth-ellis:for-let-changesCopy head branch name to clipboard

Conversation

gareth-ellis commented Mar 21, 2016

Pull Request check-list

Affected core subsystem(s)

Description of change

Uh oh!

mhdawson Mar 21, 2016

Choose a reason for hiding this comment

Uh oh!

gareth-ellis Mar 21, 2016

Choose a reason for hiding this comment

Uh oh!

Fishrock123 commented Mar 21, 2016

Uh oh!

trevnorris commented Mar 21, 2016

Uh oh!

jasnell commented Mar 21, 2016

Uh oh!

mhdawson commented Mar 21, 2016

Uh oh!

MylesBorins commented Mar 21, 2016

Uh oh!

gareth-ellis commented Mar 21, 2016

Uh oh!

MylesBorins commented Mar 21, 2016

Uh oh!

trevnorris commented Mar 21, 2016

Uh oh!

mscdex commented Mar 21, 2016

Uh oh!

MylesBorins commented Mar 21, 2016

Uh oh!

gareth-ellis commented Mar 21, 2016

Uh oh!

benjamingr commented Mar 23, 2016

Uh oh!

mhdawson commented Mar 23, 2016

Uh oh!

benjamingr commented Mar 23, 2016

Uh oh!

benjamingr commented Mar 23, 2016

Uh oh!

MylesBorins commented Mar 30, 2016

Uh oh!

jasnell commented Mar 30, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

buffer: Reverting change in let to fix regressions in buffer operations#5819
gareth-ellis wants to merge 1 commit into
nodejs:masterfrom
gareth-ellis:for-let-changes