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

idna: optimize punycode handling, add amortized allocation API, improve error reporting #653

Merged
merged 8 commits into from
Dec 1, 2020

Conversation

djc
Copy link
Contributor

@djc djc commented Nov 27, 2020

Benchmarks before:

test to_ascii_merged         ... bench:       2,102 ns/iter (+/- 309)
test to_ascii_puny_label     ... bench:       1,214 ns/iter (+/- 220)
test to_ascii_simple         ... bench:         246 ns/iter (+/- 34)
test to_unicode_ascii        ... bench:          74 ns/iter (+/- 14)
test to_unicode_merged_label ... bench:       2,127 ns/iter (+/- 400)
test to_unicode_puny_label   ... bench:       1,328 ns/iter (+/- 353)

Benchmarks after:

test to_ascii_merged         ... bench:       1,937 ns/iter (+/- 497)
test to_ascii_puny_label     ... bench:       1,147 ns/iter (+/- 254)
test to_ascii_simple         ... bench:         247 ns/iter (+/- 74)
test to_unicode_ascii        ... bench:          78 ns/iter (+/- 14)
test to_unicode_merged_label ... bench:       1,848 ns/iter (+/- 239)
test to_unicode_puny_label   ... bench:       1,158 ns/iter (+/- 312)

Across a few runs, I see performance improvements like this:

to_ascii_merged: 4%
to_ascii_puny_label: 12%
to_ascii_simple: 3%
to_unicode_ascii: -5% (but on a much smaller base)
to_unicode_merged_label: 14%
to_unicode_puny_label: 13%

This pretty much comes from the first four commits (each commit is logically separate and can be reviewed as such, although some of the first commits don't pay off in performance until the fourth one). The fifth commit adds a separate API that allows reusing allocations -- any performance benefits of this aren't reflected in the benchmarks because they only do one operation per benchmark. The final three commits instead improve error handling (although they don't change the API).

All of this comes at the cost of +300 SLOC in the idna crate.

I'm using this for my work project, so would like to get something like this merged.

@djc djc changed the title No alloc idna idna: optimize punycode handling, add amortized allocation API, improve error reporting Nov 27, 2020
@djc
Copy link
Contributor Author

djc commented Nov 27, 2020

Also happy to split this in separate PRs if that helps.

idna/src/uts46.rs Outdated Show resolved Hide resolved
@valenting
Copy link
Collaborator

Great work on this one. I'll merge it ASAP.

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.

2 participants