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

lib: replace sprintf-like format to template string literal #691

Closed
wants to merge 1 commit into from
Closed

lib: replace sprintf-like format to template string literal #691

wants to merge 1 commit into from

Conversation

yosuke-furukawa
Copy link
Member

Template String Literal is faster than sprintf-like format string.

http://jsperf.com/template-literal-vs-string-plus/2
2015-02-03 1 33 05

And, I think template string literal is easier to understand than sprintf.

localPort);
debug(
`binding to localAddress : ${localAddress} and localPort : ${localPort}`
);
Copy link
Member

Choose a reason for hiding this comment

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

This is a bit at odds with the house style. Maybe like this?

debug(`binding to localAddress: ${localAddress} ` +
      `and localPort: ${localPort}`);

@bnoordhuis
Copy link
Member

This looks like an unequivocally good change to me. If you address the nit, I'll land it. Thanks, Yosuke.

@tellnes
Copy link
Contributor

tellnes commented Feb 2, 2015

Is this rely faster? When iojs is not printing debug output, then debug will be a noop. So that boils it down to which is faster of calling a noop with multiple arguments or a noop with one string composed with template string literal.


var now = Timer.now();
debug('now: %s', now);
debug(`now : ${now}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Don’t add space before :.

@Qard
Copy link
Member

Qard commented Feb 2, 2015

It seems our linter doesn't understand template strings. It complains that there needs to be spaces before : symbols inside of the strings.

@rvagg
Copy link
Member

rvagg commented Feb 3, 2015

I wonder if util.format could be improved for the simple cases .. sounds like feature bloat has slowed it down somewhat.

And, fascinating that template strings are faster than concatenation.

One thing to consider here re perf is that all of these changes are are almost irellevant for performance because they are debug or throw situations and almost all involve using console (therefore sync) output anyway, so minor JS gains will likely be made insignificant. As a rule, I'm in favour of keeping things maintainable and grokkable over chasing tiny performance increases but for me the question is still open as to whether template strings used like this pass this bar--perhaps they do and we'll all be used to looking at them in all our code from now on?

/cc @caitp who I suspect may be interested in this change

@jbergstroem
Copy link
Member

@rvagg I think the template strings are a readability improvement.

@caitp
Copy link
Contributor

caitp commented Feb 3, 2015

And, fascinating that template strings are faster than concatenation.

I'm certainly interested in this statement, since template literals are desugared to string concatenation during parsing. Tagged templates are likely to be quite a bit slower as well, because of the rather expensive caching mechanism.

That said, go for it

@yosuke-furukawa
Copy link
Member Author

Thank you for comments.

I think template string literals have 2 pros.

  1. faster than util.format.
  2. improve readability compared to string concatenation.

So I suggest this pull request.

@bnoordhuis @charmander
Thanks for review.
When I fixed your review comments, Closure linter reports Missing space before ":" messages. the linter does not recognize ${foo}: var style strings. I reported this issue to closure linter.

Currently, I would like to replace some sprintf-like strings that are not reported by closure linter.
When the issue of closure linter is fixed or we change our linter tool, I would like to replace all sprintf-like strings to template string literals.

@sam-github
Copy link
Contributor

I'll take you word for it that string interpolation is faster in general. But the debug statements you changed actually end up as no-ops when NODE_DEBUG is not set to enable the output. Before, that mean no string formatting happened. Now, the interpolation happens even when the string is discarded, unless v8 is smart enough to realize the string is passed to a function that doesn't use it, and so avoids doing interpolation?

@caitp
Copy link
Contributor

caitp commented Feb 3, 2015

v8 is smart enough to realize the string is passed to a function that doesn't use it, and so avoids doing interpolation?

It's possible that this could happen in crankshaft (likely does not yet happen in turbofan), in hot functions. But full-codegen will not get rid of unused interpolated strings. However, string interpolation can potentially have side effects, so it's possible that those side effects would prevent them from being removed even in crankshaft

@vkurchatkin
Copy link
Contributor

Going to close this for now as it mostly changes debug messages. Also linter is not particularly happy with template strings

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants