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

Text encoding speedup #3596

Merged
merged 4 commits into from
Jan 4, 2020
Merged

Text encoding speedup #3596

merged 4 commits into from
Jan 4, 2020

Conversation

lucacasonato
Copy link
Member

I have sped up the TextEncoder for UTF-8 encoding by utilizing a encoding function from https://github.com/samthor/fast-text-encoding.

The speed improvement is about 4.5x. This is probably because there are way less allocations as the encoded bytes are written right into a Uint8Array. Maybe this can be optimized further, but it is a definite improvement over what we have now.

I did not do this for TextEncoder#encodeInto as I can not guarantee that passed Uint8Array() is long enough for the content. TextEncoder#encodeInto still uses the old encoder.

@ry
Copy link
Member

ry commented Jan 4, 2020

Great! Can you patch deno_website2 so that the benchmark displays and we can observe this improvement?

@lucacasonato
Copy link
Member Author

Great! Can you patch deno_website2 so that the benchmark displays and we can observe this improvement?

Already does. The website picks up new variants for existing benchmarks automatically. As this is just a new variation for the speed benchmarks (Execution time, Thread count, Max Memory Usage, ect.) its on there already.

BTW idk why GitHub is showing that I added the perf benchmark in this PR. I added that in #3589 so the website already has one data point for that benchmarks before this speedup.

@nayeemrmn
Copy link
Collaborator

BTW idk why GitHub is showing that I added the perf benchmark in this PR. I added that in #3589 so the website already has one data point for that benchmarks before this speedup.

You've based this on your first PR commit (fbd4bbb) as opposed to the one merged onto master (0a90094). I don't know that it makes a difference, though.

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

LGTM

@ry ry merged commit c41280a into denoland:master Jan 4, 2020
@lucacasonato lucacasonato deleted the text-encoding-speedup branch April 11, 2021 09:39
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.

None yet

3 participants