-
-
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
Merged
dcodeIO
merged 9 commits into
AssemblyScript:master
from
MaxGraey:use-templates-in-loader
Jun 13, 2020
Merged
modernize loader #1279
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
0ff6d5c
modernize loader
MaxGraey 2076a1e
revert some changes
MaxGraey 32aec8b
fix
MaxGraey 2ef43dd
refactor preInstantiate a little
MaxGraey 9429298
more
MaxGraey bd8f35c
remove unnecessary parentesies
MaxGraey 17a9101
replace str concat to interpolations
MaxGraey 80d6f93
make validateArrayInfo helper
MaxGraey a7b1287
rename validateArrayInfo to getArrayInfo
MaxGraey File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
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 likeThere 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
Uh oh!
There was an error while loading. Please reload this page.
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