-
-
Notifications
You must be signed in to change notification settings - Fork 670
modernize loader #1279
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
modernize loader #1279
Conversation
do { | ||
const last = U16[offset + CHUNKSIZE - 1]; | ||
const size = last >= 0xD800 && last < 0xDC00 ? CHUNKSIZE - 1 : CHUNKSIZE; | ||
parts.push(String.fromCharCode.apply(String, U16.subarray(offset, offset += size))); | ||
parts += String.fromCharCode.apply(String, U16.subarray(offset, offset += size)); |
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.
Are you sure that this is always faster? Would imagine that it is for smaller strings, but once parts
grows bigger, it might slow down substantially.
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.
On Chrome it should be definitely faster. See this tests:
https://twitter.com/MaxGraey/status/1232045301374111744
https://esbench.com/bench/5e540d53328a52009e8b1b22
It relate to string reversing but it also use same technique as string decoding
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.
You don't need to do String.fromCharCode.apply(String)
could simply do something like
const fromCharCode = String.fromCharCode;
fromCharCode.apply(null, theArray);
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.
Does this affect performance?
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.
I don't think so
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.
@dcodeIO I made micro benchmark which more relate to this case:
https://gist.github.com/MaxGraey/6db5c6c6d7e4a0f45154b73a5e130976
and it seems string concat approach faster in 3 times
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.
While 4K+ is already a reasonably long string, an interesting measurement there would be how long a string has to be for both approaches to execute in an equal time span, if at all. That would help to inform if it is good enough in any case, or if we'd need another code path to deal with strings longer than X.
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.
Also, the test string is exclusively ASCII currently, so it might be that engines optimize this differently than strings that have at least one non-LATIN1 character. Both cases would be interesting.
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.
Based on my benchmarks inside Node, concat
is faster than +
:
https://github.com/aminya/typescript-optimization#string--vs-concat
Thanks :) |
🎉 This PR is included in version 0.11.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
getStringImpl
by switching from collecting array parts to direct string concatenations