-
Notifications
You must be signed in to change notification settings - Fork 30k
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
Conversation
localPort); | ||
debug( | ||
`binding to localAddress : ${localAddress} and localPort : ${localPort}` | ||
); |
There was a problem hiding this comment.
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}`);
This looks like an unequivocally good change to me. If you address the nit, I'll land it. Thanks, Yosuke. |
Is this rely faster? When iojs is not printing debug output, then |
|
||
var now = Timer.now(); | ||
debug('now: %s', now); | ||
debug(`now : ${now}`); |
There was a problem hiding this comment.
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 :
.
It seems our linter doesn't understand template strings. It complains that there needs to be spaces before |
I wonder if 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 |
@rvagg I think the template strings are a readability improvement. |
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 |
Thank you for comments. I think template string literals have 2 pros.
So I suggest this pull request. @bnoordhuis @charmander Currently, I would like to replace some sprintf-like strings that are not reported by closure linter. |
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? |
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 |
Going to close this for now as it mostly changes |
Template String Literal is faster than sprintf-like format string.
http://jsperf.com/template-literal-vs-string-plus/2
And, I think template string literal is easier to understand than sprintf.