Improve sorting candidates containing numbers #13507
Merged
+283
−107
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
This `compare` function compares two strings. However, once a number is reached the numbers are compared as actual numbers instead of the string representation. E.g.: ``` p-1 p-2 p-10 p-20 ``` Will be sorted as expected in this order, instead of ``` p-1 p-10 p-2 p-20 ``` --- This should also make suggestions in the vscode extension more logical.
RobinMalfait
changed the title
RobinMalfait
changed the title
This makes the code more correct _and_ improves performance because the `Number(…)` will now always deal with numbers. On the tailwindcss.com codebase, sorting now goes from `~3.29ms` to `~3.10ms`
In this branch, it's guaranteed that numbers are _different_ which means that they are never going to be the same thus unreachable code. When we compare two strings such as: ``` foo-123-bar foo-123-baz ``` Then all characters until the last character is the same character in both positions. This means that "numbers" that are the same in the same position will be compared as strings instead of numbers. But that is fine because they are the same anyway.
This can happen if we are sorting `0123` and `123`. The `Number` representation will be equal, but the string is not. Will rarely or even never happen. But if it does, this makes it deterministic.
|
Nice — hard to tell just scanning the code but what's the behavior for strings that have numbers in the middle of them ( |
|
Something like |
thecrypticace
requested changes
Apr 12, 2024
|
I'll add some more tests cases 👍 |
thecrypticace
approved these changes
Apr 12, 2024
This also gets rid of some explanation that can now be omitted entirely.
rvanvelzen
reviewed
Apr 16, 2024
Comment on lines
+22
to
+25
| let aStart = i | ||
| let aEnd = i | ||
| let zStart = i | ||
| let zEnd = i |
There was a problem hiding this comment.
FWIW, aEnd and zEnd could have been i + 1 to prevent comparing the same offset twice.
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.
This PR adds a custom
comparefunction that compares two strings. However, once a number is reached the numbers are compared as actual numbers instead of their string representation.Before this change,
p-1 p-2 p-10 p-20would be sorted asp-1 p-10 p-2 p-20. After this change, the order isp-1 p-2 p-10 p-20.This makes the suggestions in the vscode extension more logical.
The implementation compares two strings, and as long as the strings don't contain any numbers, it compares them as strings. This way we don't pay a performance penalty for comparing strings that don't contain numbers.
Testing this change on the
tailwindcss.comcodebase where we have to store~3200unique classes the time spend on sorting the classes went from~2.35msto~3.29ms~3.10ms. This means that there is about a~1ms~0.75msperformance penalty for sorting this.Another thing we could have done is to compare using the
localeComparemethod:However, this is an order of magnitude slower than the current implementation, and sorting takes around
~30msinstead.Outstanding questions:
We could only do this for the VS Code extension and Prettier plugin because then we wouldn't have to pay the performance penalty ofThe performance is below ~1ms so good to include everywhere 👍~1ms~0.75ms. This however means that the order in the extension or the Prettier plugin would be different compared to the order in the generated CSS. Should we only do it there or everywhere and pay the~1ms~0.75mspenalty in favor of nicer consistency?