-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Use a more performant utf8 decoder algorithm. #3204
Conversation
Fixes denoland#3163 Co-authored-by: Kitson Kelly <me@kitsonkelly.com> Co-authored-by: Qwerasd <qwerasd205@users.noreply.github.com>
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.
LGTM -
Eagerly watching for perf changes to text_decoder on https://deno.land/benchmarks#exec-time
12,12,12,12,12,12,12,24,12,12,12,12, 12,24,12,12,12,12,12,12,12,24,12,12, | ||
12,12,12,12,12,12,12,36,12,36,12,12, 12,36,12,12,12,12,12,36,12,36,12,12, | ||
12,36,12,12,12,12,12,12,12,12,12,12 | ||
][state + type]; |
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.
Can't we gain even more perf by creating those long arrays outside of the function ?
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.
Can't we gain even more perf by creating those long arrays outside of the function ?
Seems like v8 optimizes it. I wrote this decode function, and in my testing I got more performance by inlining the arrays than by having them as consts defined outside of the function.
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.
Yeah, V8 optimises things like this, not sure why it is more performant this way, but likely because V8 can understand 100% that there are no mutations to this arrays because they exist in a function that is 100% knowable. In another scope, they might limit the analysis of potential mutations. They aren't like strings and the string table, and where const means const.
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.
really good to know thanks !
Fixes #3163