Benchmark var -> const before merging mass rewrites

[Fishrock123]

@nodejs/collaborators

Suggested by @mscdex in #8609 (comment)

This may sound a bit crazy, but we may want to benchmark this change. I noticed not too long ago that var => const was actually causing slowdowns and timers are definitely a hot path. I do not know if this has changed with V8 5.4 or not though.

[cjihrig]

Most of the mass rewrite is in tests. This only really needs to apply to lib/ code right?

[Fishrock123]

Probably. I wouldn't mind the tests running faster either but I doubt it would be noticeable.

[addaleax]

Btw, I’m sorry if you feel like this is all a bit much. There were a lot more people showing up than anyone anticipated, so distributing tasks was kind of an ad-hoc thing, and I think most of us see that as a really positive thing (the big turnout) – @Fishrock123 @mscdex @cjihrig @targos and everyone else who’s not busy here and reviewing: you are just the best. ❤️ </offtopic>

[mscdex]

Yes, my primary concern was about code in lib/. I will try to do some benchmarking this weekend and also see what the story is with V8 5.4.

[mcollina]

I've checked const vs var vs let in node v4 and v6 with the following script:

'use strict'

var suite = new require('benchmark').Suite()

suite.add('const', function () {
  const value = 2
  var result = 1

  for (var i = 0; i < 10000; i++) {
    result += value
  }
})

suite.add('var', function () {
  var value = 2
  var result = 1

  for (var i = 0; i < 10000; i++) {
    result += value
  }
})

suite.add('let+const', function () {
  const value = 2
  let result = 1

  for (let i = 0; i < 10000; i++) {
    result += value
  }
})

suite.on('cycle', cycle)

suite.run()

function cycle (e) {
  console.log(e.target.toString())
}

v6.6.0:

const x 156,967 ops/sec ±0.63% (90 runs sampled)
var x 156,124 ops/sec ±0.77% (88 runs sampled)
let+const x 34,882 ops/sec ±0.76% (91 runs sampled)

v4.5.0:

const x 157,721 ops/sec ±0.80% (86 runs sampled)
var x 159,177 ops/sec ±0.74% (91 runs sampled)
let+const x 9,881 ops/sec ±0.92% (87 runs sampled)

I would say using const is safe, using let should be highly discouraged.

[mscdex]

I'm not so sure it's quite that easy to benchmark as there could be many factors (e.g. assigning a constant value vs a function call return value, function (de)optimizations, etc.).

[imyller]

Do we have a reason to expect different (better) benchmark result with V8 5.4?

[ofrobots]

I would suggest the following rule of thumb, until TurboFan becomes the default optimizer (sometime next year):

• Avoid let for variables that are modified inside loops. When in doubt, stick with var.

EDIT: but measure first. In most cases it doesn't matter. Be wary of making generalizations based on micro-benchmarks.

[trevnorris]

I'd like to point out that by keeping benchmarks using the absolute latest features it makes it impossible to test them against older versions of Node. Requiring editing the file and removing the offending syntax. I'd say for benchmarks that don't involve new JS features we use the backwards compatible syntax as much as possible.

[Fishrock123]

Yeah, 100% what @trevnorris said where possible.

[fhinkel]

Let/const performance in V8 was recently improved: #9729 (comment)