util: Use of let and const. Optimized array use.

[danielsan]

Because accessing properties is more expensive than accessing variables
I moved the variables declared on lines 25 and 26 to lines 14 and 15.
Thus the previous piece of code can take advantage of those.

The push method is more expensive than array index attribution.
Since the new array length is always known, the creation of the array
with a predefined length for future positioning attribution is less
expensive than using the push method.

[cjihrig]

This could be declared with const.

[danielsan]

That is not a good idea.

1. Semantics: creating a memory reference (variable or constant) that will for sure be modified should be declared as a variable, not a constant.
2. Scope: by using let the variable objects only exists in the scope of the if statement, but that is not true when using const

[cjihrig]

1. Semantics: creating a memory reference (variable or constant) that will for sure be modified should be declared as a variable, not a constant.

The value that a const object refers to is fair game to be changed. We already do this in other places throughout the codebase.

1. Scope: by using let the variable objects only exists in the scope of the if statement, but that is not true when using const

const is also block scoped in strict mode, which all of our code is run in.

[jbergstroem]

@danielsan can you perhaps show the performance improvement in a benchmark?

[danielsan]

Benchmarks added @jbergstroem

[Trott]

@danielsan I think the benchmark @jbergstroem was looking for was a benchmark that shows the speed increase of util.format() rather than a generic push-vs.-index benchmark.

[Trott]

I'm going to run this through CI as-is just to make sure there aren't any surprising issues lurking.

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

[Trott]

Minor linting complaint on CI:

/usr/home/iojs/build/workspace/node-test-linter/lib/util.js
  18:9  error  'objects' is never modified, use 'const' instead  prefer-const

��� 1 problem (1 error, 0 warnings)

[jasnell]

let in this case is apparently not optimized by v8 just yet. /cc @bnoordhuis @ofrobots

[bnoordhuis]

Is that conjecture or did --trace_opt --trace_deopt print out something?

[jasnell]

Conjecture. I've been hearing rumors that let in for loops is not yet fully optimized by v8. Is that rumor incorrect?

[bnoordhuis]

Incorrecter with every upgrade. My rule of thumb is to prefer let/const and fall back to var only when the optimizer complains. It's easy enough to check.

[jasnell]

+1.. Good to know!
On Feb 20, 2016 7:41 AM, "Ben Noordhuis" notifications@github.com wrote:

In lib/util.js
#4678 (comment):

if (typeof f !== 'string') {

• var objects = [];
• for (var index = 0; index < arguments.length; index++) {

•  objects.push(inspect(arguments[index]));
  

• let objects = new Array(len);
• for (let i = len; i--;) {

Incorrecter with every upgrade. My rule of thumb is to prefer let/const
and fall back to var only when the optimizer complains. It's easy enough to
check.

—
Reply to this email directly or view it on GitHub
https://github.com/nodejs/node/pull/4678/files#r53552240.

[danielsan]

That has already been changed.

[danielsan]

changed from let to const on the declaration of objects variable/constant

[jasnell]

LGTM

[jasnell]

Turns out using let has a real performance cost currently. Can this be updated to remove the use of let in the for loops?

[MylesBorins]

If we are going to be avoiding let in for loops project wide I think this should also include a lint rule, which in turn should be sent upstream to eslint to allow this best practice to be adopted in the ecosystem

[jasnell]

@mscdex ... what do you think about this one? given the benchmarking you've done, is this PR something we'd want to avoid for now?

[mscdex]

@jasnell I haven't done any benchmarking around let since v8 5.0 yet. I would suggest holding off on this for the time being until we can show that let no longer has performance issues.

[MylesBorins]

what are the benchmarks on v6?