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

gareth-ellis · 2016-03-21T13:49:11Z

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

mhdawson · 2016-03-21T14:09:43Z

lib/buffer.js

@@ -292,7 +292,7 @@ Buffer.concat = function(list, length) {

  var buffer = Buffer.allocUnsafe(length);
  var pos = 0;
-  for (let i = 0; i < list.length; i++) {
+  for (i = 0; i < list.length; i++) {


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,

Fishrock123 · 2016-03-21T15:00:12Z

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

trevnorris · 2016-03-21T16:09:20Z

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.

jasnell · 2016-03-21T16:15:56Z

LGTM (with @trevnorris' nits addressed)

mhdawson · 2016-03-21T17:01:41Z

Lgtm

MylesBorins · 2016-03-21T17:37:18Z

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

@ofrobots

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

gareth-ellis · 2016-03-21T18:36:16Z

Have squashed the commits and expanded commit message.

MylesBorins · 2016-03-21T18:56:59Z

👍

trevnorris · 2016-03-21T19:15:47Z

excellent thanks

mscdex · 2016-03-21T21:22:25Z

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

MylesBorins · 2016-03-21T21:31:03Z

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

gareth-ellis · 2016-03-21T21:52:57Z

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);

benjamingr · 2016-03-23T10:08:04Z

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

mhdawson · 2016-03-23T15:23:59Z

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

benjamingr · 2016-03-23T15:42:34Z

Sure, I'll land.

@ofrobots

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

benjamingr · 2016-03-23T15:47:32Z

Landed in 443c2d5 , thanks!

@ofrobots

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

MylesBorins · 2016-03-30T19:45:37Z

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

jasnell · 2016-03-30T19:49:17Z

No objections

@ofrobots

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

@ofrobots

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

gareth-ellis commented Mar 21, 2016

mhdawson Mar 21, 2016

gareth-ellis Mar 21, 2016

Fishrock123 commented Mar 21, 2016

trevnorris commented Mar 21, 2016

jasnell commented Mar 21, 2016

mhdawson commented Mar 21, 2016

MylesBorins commented Mar 21, 2016

gareth-ellis commented Mar 21, 2016

MylesBorins commented Mar 21, 2016

trevnorris commented Mar 21, 2016

mscdex commented Mar 21, 2016

MylesBorins commented Mar 21, 2016

gareth-ellis commented Mar 21, 2016

benjamingr commented Mar 23, 2016

mhdawson commented Mar 23, 2016

benjamingr commented Mar 23, 2016

benjamingr commented Mar 23, 2016

MylesBorins commented Mar 30, 2016

jasnell commented Mar 30, 2016

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

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

Conversation

gareth-ellis commented Mar 21, 2016

Pull Request check-list

Affected core subsystem(s)

Description of change

mhdawson Mar 21, 2016

Choose a reason for hiding this comment

gareth-ellis Mar 21, 2016

Choose a reason for hiding this comment

Fishrock123 commented Mar 21, 2016

trevnorris commented Mar 21, 2016

jasnell commented Mar 21, 2016

mhdawson commented Mar 21, 2016

MylesBorins commented Mar 21, 2016

gareth-ellis commented Mar 21, 2016

MylesBorins commented Mar 21, 2016

trevnorris commented Mar 21, 2016

mscdex commented Mar 21, 2016

MylesBorins commented Mar 21, 2016

gareth-ellis commented Mar 21, 2016

benjamingr commented Mar 23, 2016

mhdawson commented Mar 23, 2016

benjamingr commented Mar 23, 2016

benjamingr commented Mar 23, 2016

MylesBorins commented Mar 30, 2016

jasnell commented Mar 30, 2016