Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src,tools: use template literals #5778

Closed
wants to merge 1 commit into from
Closed

Conversation

Trott
Copy link
Member

@Trott Trott commented Mar 18, 2016

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.

@Trott Trott added tools Issues and PRs related to the tools directory. lts-watch-v4.x lib / src Issues and PRs related to general changes in the lib or src directory. labels Mar 18, 2016
@cjihrig
Copy link
Contributor

cjihrig commented Mar 18, 2016

LGTM


rules:
# suggest using template literals instead of string concatenation
prefer-template: 2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing new line at EOF

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@targos
Copy link
Member

targos commented Mar 18, 2016

One nit, but LGTM if CI is happy

@jasnell
Copy link
Member

jasnell commented Mar 18, 2016

LGTM

@williamkapke
Copy link
Contributor

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
Copy link
Member

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

Convert string concatenation to template literals. Enforce with lint
rule.
@Trott
Copy link
Member Author

Trott commented Mar 19, 2016

@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
Copy link
Member

targos commented Mar 19, 2016

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

@jbergstroem
Copy link
Member

LGTM

@benjamingr
Copy link
Member

@benjamingr
Copy link
Member

Cheers f158a86

@benjamingr benjamingr closed this Mar 20, 2016
benjamingr pushed a commit that referenced this pull request Mar 20, 2016
Convert string concatenation to template literals. Enforce with lint
rule.

PR-URL: #5778
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
@williamkapke
Copy link
Contributor

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

@Trott
Copy link
Member Author

Trott commented Mar 21, 2016

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.

benjamingr added a commit to benjamingr/io.js that referenced this pull request Mar 22, 2016
Prefer the use of template string literals over string concatenation
in the dns module, makes dns consistent with other modules basically
doing nodejs#5778 for it.

PR-URL: nodejs#5809
Reviewed-By: James M Snell <jasnell@gmail.com>
benjamingr added a commit that referenced this pull request Mar 22, 2016
Prefer the use of template string literals over string concatenation
in the dns module, makes dns consistent with other modules basically
doing #5778 for it.

PR-URL: #5809
Reviewed-By: James M Snell <jasnell@gmail.com>
@Fishrock123
Copy link
Contributor

This rule will be completely invalid as of #5103

@benjamingr
Copy link
Member

@Fishrock123 mind elaborating? Which rule?

@Trott
Copy link
Member Author

Trott commented Mar 22, 2016

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

Fishrock123 pushed a commit that referenced this pull request Mar 22, 2016
Convert string concatenation to template literals. Enforce with lint
rule.

PR-URL: #5778
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>

Conflicts:
	src/.eslintrc
	src/node.js
Fishrock123 pushed a commit that referenced this pull request Mar 22, 2016
Prefer the use of template string literals over string concatenation
in the dns module, makes dns consistent with other modules basically
doing #5778 for it.

PR-URL: #5809
Reviewed-By: James M Snell <jasnell@gmail.com>
Fishrock123 pushed a commit that referenced this pull request Mar 23, 2016
Prefer the use of template string literals over string concatenation
in the dns module, makes dns consistent with other modules basically
doing #5778 for it.

PR-URL: #5809
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins
Copy link
Contributor

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?

MylesBorins pushed a commit that referenced this pull request Mar 30, 2016
Prefer the use of template string literals over string concatenation
in the dns module, makes dns consistent with other modules basically
doing #5778 for it.

PR-URL: #5809
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott
Copy link
Member Author

Trott commented Mar 30, 2016

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
Copy link
Contributor

thanks @Trott

MylesBorins pushed a commit that referenced this pull request Mar 30, 2016
Prefer the use of template string literals over string concatenation
in the dns module, makes dns consistent with other modules basically
doing #5778 for it.

PR-URL: #5809
Reviewed-By: James M Snell <jasnell@gmail.com>
Trott added a commit to Trott/io.js that referenced this pull request Apr 3, 2016
Convert string concatenation to template literals. Enforce with lint
rule.

PR-URL: nodejs#5778
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
MylesBorins pushed a commit that referenced this pull request Apr 4, 2016
Convert string concatenation to template literals. Enforce with lint
rule.

PR-URL: #5778
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
@MylesBorins MylesBorins mentioned this pull request Apr 11, 2016
@Trott Trott deleted the prefer-template branch January 13, 2022 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants