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

sort: slices.Sort is slightly slower than sort.Strings on some sorted inputs #60777

Closed
qiulaidongfeng opened this issue Jun 14, 2023 · 13 comments
Assignees
Labels
NeedsFix The path to resolution is known, but the work has not been done. Performance
Milestone

Comments

@qiulaidongfeng
Copy link
Contributor

What version of Go are you using (go version)?

$ go version devel go1.21-39effbc105 Fri Jun 9 04:07:29 2023 +0000 windows/amd64

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
set GOOS=windows
set GOARCH=amd64
set GOAMD64=v2

What did you do?

Use the following benchmark code

code
package main

import (
//"slices"
"sort"
"strconv"
"testing"
)

var s []string

func init() {
const n = 10000000
s = make([]string, n)
for i := 0; i < n; i++ {
s[i] = strconv.Itoa(i)
}
}

func BenchmarkStringSort(b *testing.B) {
for i := 0; i < b.N; i++ {
sort.Strings(s)
}
}

// func BenchmarkStringSort(b *testing.B) {
// for i := 0; i < b.N; i++ {
// slices.Sort(s)
// }
// }

What did you expect to see?

Expected document to be successfully validated

Note: consider using the new slices.Sort function, which runs faster

What did you see instead?

│ sort.txt │ slices.txt │ │ sec/op │ sec/op vs base │ StringSort-4 123.7m ± 2% 134.0m ± 1% +8.36% (p=0.002 n=10)

The test results do not match the document, which says slices is faster, but the test found that sort is faster.

@qiulaidongfeng qiulaidongfeng changed the title sort: The benchmark test does not match the document, sort is faster,In GOAMD64=v2 Sort: The benchmark test does not match the document, sort is 8% faster, in GOAMD64=v2 Jun 14, 2023
@randall77 randall77 changed the title Sort: The benchmark test does not match the document, sort is 8% faster, in GOAMD64=v2 sort: the benchmark test does not match the document, sort is 8% faster, with GOAMD64=v2 Jun 14, 2023
@randall77
Copy link
Contributor

The test results do not match the document, which says slices is faster, but the test found that sort is faster.

Which document are you referring to?

@ianlancetaylor
Copy link
Contributor

CC @eliben

@ianlancetaylor
Copy link
Contributor

That's not a great benchmark, because it just calls the sorting function over and over again on the same slice. After the first call, the slice will be fully sorted. So you aren't really testing how long it takes to sort, you are testing how long it takes to determine that the slice is already sorted.

There are some existing benchmarks in slices/sort_benchmark_test.go and sort/sort_test.go that don't have that problem.

@qiulaidongfeng
Copy link
Contributor Author

Document in https://pkg.go.dev/sort@master#Strings
I use the following benchmarks to sort different slices in each loop:

code package main

import (
//"slices"
"sort"
"strconv"
"testing"
)

const n = 100000

func BenchmarkStringSort(b *testing.B) {
for i := 0; i < b.N; i++ {
s := make([]string, n)
for i := 0; i < n; i++ {
s[i] = strconv.Itoa(i)
}
sort.Strings(s)
}
}

// func BenchmarkStringSort(b *testing.B) {
// for i := 0; i < b.N; i++ {
// s := make([]string, n)
// for i := 0; i < n; i++ {
// s[i] = strconv.Itoa(i)
// }
// slices.Sort(s)
// }
// }

│ sort.txt │ slices.txt │ │ sec/op │ sec/op vs base │ StringSort-4 38.63m ± 1% 36.23m ± 2% -6.23% (p=0.000 n=10)

Note: consider using the new slides. Sort function, which runs faster

This document is easily understood as calling slices.Sort on the same slice (using [] string as an example) is faster than calling sort.Strings.

I think this part of the document should be changed to: "Note: Consider using the newer slices.Sort function to run faster on Not sorted slices", It's much less likely to be misunderstood.

@mitar
Copy link
Contributor

mitar commented Jun 14, 2023

Which document are you referring to?

I think author means "documentation".

@eliben
Copy link
Member

eliben commented Jun 14, 2023

In my measurements, slices.Sort is 15-20% faster on a shuffled slice of strings (this is what I get by running the in-tree benchmarks @iant mentioned). For a presorted slice, I see measurement noise - either function is ~1% faster depending on the run.

Both functions end up in the same code-generated implementation, and the difference between them is the cost of dispatch (StringSlice interface for sort.Strings and generic dispatch for cmp.Ordered for slices.Sort) so I don't really see why sorting a sorted slice would be consistently faster for sort.Strings.

@zhangyunhao116

@zhangyunhao116
Copy link
Contributor

zhangyunhao116 commented Jun 15, 2023

For this benchmark, the sort.Strings is slightly faster.

func sortedStrings() []string {
	const N = 10000
	x := make([]string, N)
	for i := 0; i < N; i++ {
		x[i] = strconv.Itoa(i)
	}
	slices.Sort(x)
	return x
}

func BenchmarkSlices(b *testing.B) {
	x := sortedStrings()
	b.ResetTimer()
	for i := 0; i < b.N; i++ {
		slices.Sort(x)
	}
}

func BenchmarkSort(b *testing.B) {
	x := sortedStrings()
	b.ResetTimer()
	for i := 0; i < b.N; i++ {
		sort.Strings(x)
	}
}

We can see the diff from its flame graph. I think the reason is that the slices use cmp.Less, which consumes too much CPU on the isNaN. IMO, we can optimize this pattern in the compiler, there are many types like the string that don't require the isNaN.

Screenshot 2023-06-15 at 13 02 03

@bronze1man
Copy link
Contributor

bronze1man commented Jun 15, 2023

"IMOP, we can optimize this pattern in the compiler, there are many types like the string that don't require the isNaN."
Can golang just add compile time type check (rust called "Zero Cost Abstractions") to only check isNan with float32 or float64? Or the compiler can transform runtime type check to compile time type check?

@randall77
Copy link
Contributor

That just looks like a missing optimization. x==x should always return true for strings, and it doesn't.

func f(x string) bool {
	return x == x
}

That should compile to the equivalent of return true, but it doesn't. It ends up calling memequal (which does shortcut immediately to return true, but the call itself is still there).

@eliben
Copy link
Member

eliben commented Jun 15, 2023

For this benchmark, the sort.Strings is slightly faster.

func sortedStrings() []string {
	const N = 10000
	x := make([]string, N)
	for i := 0; i < N; i++ {
		x[i] = strconv.Itoa(i)
	}
	slices.Sort(x)
	return x
}

func BenchmarkSlices(b *testing.B) {
	x := sortedStrings()
	b.ResetTimer()
	for i := 0; i < b.N; i++ {
		slices.Sort(x)
	}
}

func BenchmarkSort(b *testing.B) {
	x := sortedStrings()
	b.ResetTimer()
	for i := 0; i < b.N; i++ {
		sort.Strings(x)
	}
}

Yes, this data is different so I see slightly different results. sort.Strings is faster by 1-2%

@randall77 makes a good point about how this can be fixed.

In the meantime, I'm not sure whether the doc comment should be modified. The new slices.Sort is certainly much faster on "real" inputs; it seems like for some subset of pre-sorted inputs it can be slightly slower. We could qualify the comment from "runs faster" to "runs faster in most cases", but I'm not sure it's worth it.

@gopherbot
Copy link

Change https://go.dev/cl/503795 mentions this issue: cmd/compile: optimize s==s for strings

@gopherbot
Copy link

Change https://go.dev/cl/503815 mentions this issue: slices: add sort benchmark for sorted strings

gopherbot pushed a commit that referenced this issue Jun 15, 2023
For #60777

Change-Id: I424535ce6454156c61af2f299228630ee304d165
Reviewed-on: https://go-review.googlesource.com/c/go/+/503815
Reviewed-by: Keith Randall <khr@google.com>
Run-TryBot: Eli Bendersky <eliben@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
Reviewed-by: Eli Bendersky <eliben@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@dmitshur dmitshur added the NeedsFix The path to resolution is known, but the work has not been done. label Jun 19, 2023
@dmitshur dmitshur added this to the Go1.21 milestone Jun 19, 2023
@eliben eliben changed the title sort: the benchmark test does not match the document, sort is 8% faster, with GOAMD64=v2 sort: slices.Sort is slightly slower than sort.Strings on some sorted inputs Jun 20, 2023
@eliben
Copy link
Member

eliben commented Jun 20, 2023

I've updated the issue title to better reflect the current understanding. @randall77 's https://go.dev/cl/503795 fixes this, but we're not sure whether this fix is worth it for inclusion in 1.21 or will have to wait for 1.22

In the meantime, my inclination is not to do anything about the documentation, given the special circumstances where it's inaccurate (only for sorted inputs, and even then, only for some sorted inputs). Happy to hear other opinions

@dmitshur dmitshur modified the milestones: Go1.21, Go1.22 Jun 27, 2023
eric pushed a commit to fancybits/go that referenced this issue Sep 7, 2023
For golang#60777

Change-Id: I424535ce6454156c61af2f299228630ee304d165
Reviewed-on: https://go-review.googlesource.com/c/go/+/503815
Reviewed-by: Keith Randall <khr@google.com>
Run-TryBot: Eli Bendersky <eliben@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
Reviewed-by: Eli Bendersky <eliben@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
eric pushed a commit to fancybits/go that referenced this issue Sep 7, 2023
For golang#60777

Change-Id: I424535ce6454156c61af2f299228630ee304d165
Reviewed-on: https://go-review.googlesource.com/c/go/+/503815
Reviewed-by: Keith Randall <khr@google.com>
Run-TryBot: Eli Bendersky <eliben@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
Reviewed-by: Eli Bendersky <eliben@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix The path to resolution is known, but the work has not been done. Performance
Projects
None yet
Development

No branches or pull requests

9 participants