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: prevent allocations (mainly) in Punycode decoding #635

Closed
wants to merge 5 commits into from

Conversation

djc
Copy link
Contributor

@djc djc commented Aug 25, 2020

No description provided.

@codecov-commenter
Copy link

codecov-commenter commented Aug 25, 2020

Codecov Report

Merging #635 into master will increase coverage by 0.21%.
The diff coverage is 80.24%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #635      +/-   ##
==========================================
+ Coverage   81.09%   81.31%   +0.21%     
==========================================
  Files          22       22              
  Lines        3512     3537      +25     
==========================================
+ Hits         2848     2876      +28     
+ Misses        664      661       -3     
Impacted Files Coverage Δ
url/src/lib.rs 67.13% <0.00%> (-0.58%) ⬇️
idna/src/uts46.rs 94.87% <75.00%> (+3.13%) ⬆️
idna/tests/unit.rs 100.00% <100.00%> (ø)
url/tests/unit.rs 99.65% <100.00%> (+<0.01%) ⬆️
idna/src/punycode.rs 91.05% <0.00%> (+0.81%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8dedda4...5c91e06. Read the comment docs.

@djc
Copy link
Contributor Author

djc commented Aug 25, 2020

@valenting can you review the API introduced in 5c91e06? (Feel free to look at the other stuff too, but it's a bit gnarly and only amounts to a modest performance improvement for more complex invocations of to_unicode().)

@valenting
Copy link
Collaborator

@valenting can you review the API introduced in 5c91e06? (Feel free to look at the other stuff too, but it's a bit gnarly and only amounts to a modest performance improvement for more complex invocations of to_unicode().)

I think it's generally OK. As a side note, do we use Idna.normalized for anything? Or can it be just a local buffer?

@djc
Copy link
Contributor Author

djc commented Aug 25, 2020

What do you mean by "local buffer"? processing() needs a local buffer. Keeping it in Idna amortizes the cost of the allocation over all calls to Idna::to_ascii() and Idna::to_unicode().

@valenting
Copy link
Collaborator

What do you mean by "local buffer"? processing() needs a local buffer. Keeping it in Idna amortizes the cost of the allocation over all calls to Idna::to_ascii() and Idna::to_unicode().

In the sense that it doesn't seem to be used outside of processing(). But maybe I'm missing something.

@djc
Copy link
Contributor Author

djc commented Aug 26, 2020

No, that is correct. I just though amortizing the cost of allocating that buffer would be better for performance than allocating it locally for every call to processing().

@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably 3cc7e69) made this pull request unmergeable. Please resolve the merge conflicts.

@djc djc mentioned this pull request Oct 19, 2020
@djc
Copy link
Contributor Author

djc commented Oct 19, 2020

Newer version in #645.

@djc djc closed this Oct 19, 2020
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.

4 participants