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

cmd/compile: intrinsify cmp.Compare on common types such as strings #71270

Open
mvdan opened this issue Jan 14, 2025 · 10 comments
Open

cmd/compile: intrinsify cmp.Compare on common types such as strings #71270

mvdan opened this issue Jan 14, 2025 · 10 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Milestone

Comments

@mvdan
Copy link
Member

mvdan commented Jan 14, 2025

#61725 optimized strings.Compare, which is great, but it did not optimize cmp.Compare[string], which is otherwise equivalent.

This is leading to users learning that they should avoid cmp.Compare[string] for the sake of performance, even writing linters for it like https://github.com/tklauser/lintcomparestrings, which in my opinion is really unfortunate. For the same reason, I find changes like https://go-review.googlesource.com/c/go/+/642038 unfortunate and unnecessary.

The compiler should be clever enough to optimize the generic cmp.Compare function just as well as specialized functions such as strings.Compare or any others that might exist for common comparable types such as integers. Then the developers don't have to remember facts about which one of them is faster.

Personally, I also find it pretty nice to consistently use cmp.Compare. Needing to mix different compare functions in an expression for the sake of performance is a bit odd.

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jan 14, 2025
@mvdan
Copy link
Member Author

mvdan commented Jan 14, 2025

Even though this change may not seem hugely important or urgent, I would still argue for doing it sooner than later, before it's a "well known fact" that cmp.Compare is slow for common types and should be avoided.

@mknyszek mknyszek added this to the Unplanned milestone Jan 14, 2025
@mknyszek
Copy link
Contributor

CC @golang/compiler

@mknyszek mknyszek added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 14, 2025
@prattmic
Copy link
Member

prattmic commented Jan 14, 2025

Is adding a type switch to the beginning of cmp.Compare sufficient to get good results, or does that not fully simplify in the compiler?

func Compare[T Ordered](x, y T) int {
	switch any(x).(type) {
	case string:
		return bytealg.CompareString(any(x).(string), any(y).(string))
	}
	...

@mateusz834
Copy link
Member

@prattmic #59591 ?

@adonovan
Copy link
Member

Is adding a type switch to the beginning of cmp.Compare sufficient to get good results...?

It actually makes it worse: the usual call boils down to runtime.cmpstring, but the switch defeats inlining.

@fengyoulin
Copy link
Contributor

CL 578835 removed the redundant calls to runtime.cmpstring, but the redundant tests and branches cannot be removed. Maybe it's worth doing.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/647235 mentions this issue: cmd/compile: intrinsify cmp.Compare for string type

@randall77
Copy link
Contributor

I have a separate set of CLs for removing the test+branch stuff, not quite ready to mail yet.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/648336 mentions this issue: cmd/compile: remove SignInt when applied to cmpstring

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/648355 mentions this issue: cmp,cmd/compile: make cmp.Compare fast for strings

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Projects
None yet
Development

No branches or pull requests

8 participants