Implementing linear time suffix trimming #7
Merged
Conversation
This file contains 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
sindresorhus
requested changes
Feb 14, 2017
index.js
Outdated
| if (a.length > b.length) { | ||
| a = b; | ||
| b = swap; | ||
| } |
There was a problem hiding this comment.
Can you add a short code comment on why you're swapping here?
index.js
Outdated
| @@ -19,6 +26,15 @@ module.exports = function (a, b) { | |||
| return aLen; | |||
| } | |||
|
|
|||
| while (aLen > 0 && (a.charCodeAt(~-aLen) === b.charCodeAt(~-bLen))) { | |||
There was a problem hiding this comment.
Can you add a comment about what this does (suffix trimming)?
|
I've added some comments. Note also that swapping |
sindresorhus
approved these changes
Feb 14, 2017
|
This is amazing. Thank you @Yomguithereal :) |
|
I'll come back when I fix my prefix trimming (if this is still useful performance-wise) & if I find evidence that caching the longest or shortest or both char codes can be useful or not. |
|
Pretty good improvement actually: cf30e86 🎉 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Hello again @sindresorhus. This PR adds linear time suffix trimming to your function. This should give you an edge (at least it did on the benchmark I ran), but I advise you run your own just to be sure (maybe with more use cases and swapping the strings already present).
The idea is to trim the suffix of both strings linearly until they start to differ.
The strange
~-operation used here is the bitwise way to perform a-1operation on a number. It speeds up the computation a bit on my benchmarks.I can also implement prefix trimming but while writing this PR, I noticed that I did something wrong and my prefix trimming is actually erroneous so I need to fix it. However, since it means performing some more arithmetical operations, I fear it could slow down things rather than improving the performance of the function. I will come back later when my function is fixed to tell you if it's worth it to add prefix trimming then.
Have a nice day.