Add a fast path returning Cow::Borrowed for ASCII-only prepping#4
Merged
Conversation
Owner
|
I don't think there's any reason those functions couldn't have a Cow::Borrowed fast path, for lower case ASCII for example. |
Contributor
Author
|
Indeed, I’ve added the fast paths instead. I will benchmark these changes, although we are already quite a bit faster than libidn for instance. Edit: for ASCII-only JIDs already normalized in the jid crate, this reduces the parsing time between -69.6% for the shortest possible JIDs, to -98.5% for the longest possible JIDs. And for the ones which need normalization, I measured around +6% which is quite acceptable imo. |
For already normalized JIDs, this reduces the parsing time by -69.6% to -98.5% depending on the length of the JID, and only increases it for other JIDs by +5.8% which is quite acceptable in that uncommon case.
linkmauve
changed the title
Owner
|
Thanks! |
Owner
|
Want me to cut a release or are there more changes you're planning on making? |
Contributor
Author
|
After #5 I think you can make a release, I have implemented it in the jid crate and haven’t found any issue so far. |
poliorcetics
pushed a commit
to JustRustThings/xmpp-rs
that referenced
this pull request
Jul 24, 2023
stringprep can make transformations to a JID, the most well-known one is making the nodepart and domainpart lowercase but it does much more than that. It is extremely common to have to validate already-normalised JIDs though, and since sfackler/rust-stringprep#4 this is exactly what the stringprep crate does, by returning Cow::Borrowed() for common ASCII-only cases. This commit further reduces time spent by an additional -15%..-58% when already using this stringprep improvement, in addition to the 89.5%..98.5% change brought by this improvement (and +1.3% total when the JID isn’t normalised yet). For instance, my own full JID parses in 1.83 µs before these changes, 132 ns with just the stringprep optimisation, and 46 ns with also this commit, on an i7-8700K.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
In all three of nodeprep, nameprep and resourceprep, there was never a case where a
Cow::Borrowed()was generated, so let’s simplify the API for everyone.This is a breaking change in the API.