Replace sort.Strings and sort.Ints with faster slices.Sort#11318
Merged
codesome merged 2 commits intoprometheus:mainfrom Sep 30, 2022
Merged
Replace sort.Strings and sort.Ints with faster slices.Sort#11318codesome merged 2 commits intoprometheus:mainfrom
codesome merged 2 commits intoprometheus:mainfrom
Conversation
Use new experimental package `golang.org/x/exp/slices`. slices.Sort works on values that are directly comparable, like strings, so avoids the overhad of an interface call to `.Less()`. Left tests unchanged, because they don't need the speed and it may be a cross-check that slices.Sort gives the same answer. Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
Use new experimental package `golang.org/x/exp/slices`. slices.Sort works on values that are directly comparable, like ints, so avoids the overhad of an interface call to `.Less()`. Left tests unchanged, because they don't need the speed and it may be a cross-check that slices.Sort gives the same answer. Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
87573d8 to
dbad9ad
Compare
codesome
approved these changes
Sep 30, 2022
bboreham
added a commit
to bboreham/prometheus
that referenced
this pull request
Oct 1, 2022
This call was added by PR prometheus#11075 merged before prometheus#11318 which changed all similar calls to `sort.Sort` into a faster one. Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
This was referenced Oct 1, 2022
Closed
LeviHarrison
pushed a commit
that referenced
this pull request
Oct 1, 2022
Merged
3 tasks
charleskorn
added a commit
to grafana/mimir
that referenced
this pull request
Dec 12, 2022
#3639 (comment), prometheus/prometheus#11054, and prometheus/prometheus#11318 all show substantial performance improvements when using slices.Sort over sort.Strings. I've also added a linting rule to catch usage of sort.Strings and sort.Ints in the future. (We don't currently use sort.Ints anywhere.) We could also replace sort.Float64s with slices.Sort, however, this requires more careful consideration as the documentation for slices.Sort warns that it may not handle NaN values correctly.
charleskorn
added a commit
to grafana/mimir
that referenced
this pull request
Dec 15, 2022
#3639 (comment), prometheus/prometheus#11054, and prometheus/prometheus#11318 all show substantial performance improvements when using slices.Sort over sort.Strings. I've also added a linting rule to catch usage of sort.Strings and sort.Ints in the future. (We don't currently use sort.Ints anywhere.) We could also replace sort.Float64s with slices.Sort, however, this requires more careful consideration as the documentation for slices.Sort warns that it may not handle NaN values correctly.
pracucci
pushed a commit
to grafana/mimir
that referenced
this pull request
Dec 15, 2022
* Use slices.Sort instead of sort.Strings or sort.Ints. #3639 (comment), prometheus/prometheus#11054, and prometheus/prometheus#11318 all show substantial performance improvements when using slices.Sort over sort.Strings. I've also added a linting rule to catch usage of sort.Strings and sort.Ints in the future. (We don't currently use sort.Ints anywhere.) We could also replace sort.Float64s with slices.Sort, however, this requires more careful consideration as the documentation for slices.Sort warns that it may not handle NaN values correctly. * Remove unnecessary comment. * Rename benchmarks file. * Add benchmark for BinaryReader.LabelNames(). * Add benchmark for LabelValues(). * Modify mergeExemplarQueryResponses benchmark to exercise merging exemplars with different sets of labels.
masonmei
pushed a commit
to udmire/mimir
that referenced
this pull request
Dec 16, 2022
…fana#3690) * Use slices.Sort instead of sort.Strings or sort.Ints. grafana#3639 (comment), prometheus/prometheus#11054, and prometheus/prometheus#11318 all show substantial performance improvements when using slices.Sort over sort.Strings. I've also added a linting rule to catch usage of sort.Strings and sort.Ints in the future. (We don't currently use sort.Ints anywhere.) We could also replace sort.Float64s with slices.Sort, however, this requires more careful consideration as the documentation for slices.Sort warns that it may not handle NaN values correctly. * Remove unnecessary comment. * Rename benchmarks file. * Add benchmark for BinaryReader.LabelNames(). * Add benchmark for LabelValues(). * Modify mergeExemplarQueryResponses benchmark to exercise merging exemplars with different sets of labels.
This was referenced Jul 8, 2023
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.
Use new experimental package
golang.org/x/exp/slices.slices.Sort works on values that are directly comparable, like strings, so avoids the overhad of an interface call to
.Less().Left tests unchanged, because they don't need the speed and it may be a cross-check that slices.Sort gives the same answer.
Follows on from #11054.