src,tools: use template literals

[Trott]

Pull Request check-list

• 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][0]?
• 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)?

Affected core subsystem(s)

src, tools

Description of change

Convert string concatenation to template literals. Enforce with lint
rule.

This came out of the conversation around #5762. If this is not objectionable, then the idea would be to enable the prefer-template rule on other parts of the codebase, moving it incrementally to using template literals instead of string concatenation.

[cjihrig]

LGTM

[targos]

Missing new line at EOF

[Trott]

Rebased, added newline per @targos, and force pushed.

[targos]

One nit, but LGTM if CI is happy

[jasnell]

LGTM

[williamkapke]

Many of these are only doing the string templating at the beginning or at the end. Maybe this has changed and I missed it- but I thought doing concatenation was faster.

I admit- these paths are probably not hit too much, but it seems that ALL thing performance get nits ;)

[benjamingr]

None of the paths are hot and I think template strings except where justified specifically by perf makes sense. LGTM and +1

[Trott]

@williamkapke wrote:

I thought doing concatenation was faster

With the caveat that I may simply be doing the benchmark wrong, my tests are showing that template literals are faster.

Here's the code. (You'll have to npm install benchmark to get the Benchmark.js module.)

'use strict';

const Benchmark = require('benchmark');
const suite = new Benchmark.Suite;

const conclusion = 'and made quite a dish!';

suite.add('string concatenation', () => {
  `The fish was delish, ${conclusion}` ===
    'The fish was delish, and made quite a dish!';
});

suite.add('template literal', () => {
    'The fish was delish, ' + conclusion ===
      'The fish was delish, and made quite a dish!';
});

suite.on('cycle', function(event) {
  console.log(String(event.target));
});

suite.run();

And here are the results I'm getting with Node.js v5.9.0:

string concatenation x 6,529,708 ops/sec ±1.23% (87 runs sampled)
template literal x 7,726,309 ops/sec ±1.08% (88 runs sampled)

Not that it really matters because, as @benjamingr points out, none of this is in a hot path.

but it seems that ALL thing performance get nits

That has not been my experience. On the contrary, PRs with micro-optimizations on non-hot-path code and with no benchmark showing effectiveness are usually rejected on the grounds that they cause churn and reduce code readability for no measurable benefit.

[targos]

Template strings should be equally fast. They are desugared to string concatenation by V8's parser

[jbergstroem]

LGTM

[benjamingr]

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

[benjamingr]

Cheers f158a86

[williamkapke]

@Trott @targos Super awesome to know. thanks++

[Trott]

Heh, whoops, looks like I flipped the labels in my test. What I have listed for string concatenation was actually for template literals and vice versa. String concatenation seems to be somewhere between 9% and 18% faster, at least on the machines I have available to test on, and using Node.js 5.9.0. Not enough to worry about for things that only happen once, but probably enough to think twice about extreme hot-path code.

[Fishrock123]

This rule will be completely invalid as of #5103

[benjamingr]

@Fishrock123 mind elaborating? Which rule?

[Trott]

@Fishrock123 I checked out your cleanup-node.js branch, rebased against current master (which has this lint rule), and ran make jslint. It found no errors. I'm not sure what you mean by the rule being "completely invalid". My guess was "This PR is going to violate the lint rule" but that does not appear to be the case.

[MylesBorins]

I'm not 100% convinced this should be backported. @Trott it isn't landing cleanly, would you be willing to manually backport, do you think it is necessary?

[Trott]

Willing to: Sure.

Necessary: Meh, could go either way.

Here's the PR if you want it: #5960

(If you're not going to use it, just go ahead and close it.)

[MylesBorins]

thanks @Trott