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

strings: maybe strings.Compare should be optimized #50167

Closed
go101 opened this issue Dec 14, 2021 · 18 comments
Closed

strings: maybe strings.Compare should be optimized #50167

go101 opened this issue Dec 14, 2021 · 18 comments

Comments

@go101
Copy link

go101 commented Dec 14, 2021

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

$ go version
go version go1.17.5 linux/amd64

Does this issue reproduce with the latest release?

Yes

What did you do?

https://sourcegraph.com/search?q=context:global+switch+strings.Compare+lang:Go+&patternType=literal

What did you expect to see?

The strings.Compare should do one comparison for all cases.

What did you see instead?

The strings.Compare does two comparisons for some cases.

https://github.com/golang/go/blob/go1.17.5/src/strings/compare.go#L13

@seankhliao
Copy link
Member

The comment in the code explains why not

@go101
Copy link
Author

go101 commented Dec 15, 2021

I'm aware of that comment. But I think it should be optimized, as many projects use the function often.

@go101
Copy link
Author

go101 commented Dec 15, 2021

@seankhliao please reopen this issue.

@seankhliao
Copy link
Member

Many projects writing poor code doesn't mean we should optimize it, especially when we're actively trying to discourage it.

@go101
Copy link
Author

go101 commented Dec 15, 2021

Such code in many projects is not poor at all.

@go101
Copy link
Author

go101 commented Dec 15, 2021

If you think they are poor, please show the not-poor way.

@ianlancetaylor
Copy link
Contributor

The comment explicitly says that we don't want to encourage people to use strings.Compare. That hasn't changed. We expect code to compare strings using operators like == and <. Those are the cases that we should optimize (in the compiler), not the strings.Compare function. (And if we do optimize those cases in the compiler, then perhaps strings.Compare will be optimized too as a side-effect.)

@go101
Copy link
Author

go101 commented Dec 15, 2021

I understand this. But there are situations in which strings.Compare should be used and the strings.Compare is expected to be implemented performantly.

In other words, I don't think the comment is reasonable.

bytes.Compare has made the optimization, so I think it is easy for strings.Compare too.

More specifically, there are many uses cases shown in https://sourcegraph.com/search?q=context:global+switch+strings.Compare+lang:Go+&patternType=literal are like:

func foo(x, y string) {
	switch strings.Compare(x, y) {
	case -1:
		// do something 1
	case 0:
		// do something 2
	case 1:
		// do something 3
	}
}

I think this is a common use case.

@ianlancetaylor
Copy link
Contributor

Instead of worrying about strings.Compare, let's make this efficient. If we do that strings.Compare will become more efficient as a side-effect.

func foo(x, y string) {
    switch {
    case x < y:
        // do something 1
    case x == y:
        // do something 2
    default:
        // do something 3
    }
}

@go101
Copy link
Author

go101 commented Dec 16, 2021

So for some cases, even if the operator way is not efficient, it is still recommended to use this way, by making the other way also inefficient deliberately?

@ianlancetaylor
Copy link
Contributor

I would say, instead, focus on making the more readable approach efficient. Don't focus on making the less readable approach efficient. We aren't making strings.Compare deliberately inefficient; we're making it the same as the more readable approach.

@go101
Copy link
Author

go101 commented Dec 16, 2021

Readability might be a subjective word. Personally, I don't think there is a readability difference between the two ways.

The two ways are not overlapped fully. There are situations in which using strings.Compare (assumed it is implemented normally as expected) is more efficient, and there are also situations in which using operators is more efficient. I really don't understand why to deliberately prefer one to the other.

@go101
Copy link
Author

go101 commented Dec 16, 2021

And personally, the role of recommending the operator way to the strings.Compare way for some situations is go vet, instead of the standard packages.

@go101
Copy link
Author

go101 commented Dec 16, 2021

And the third, the current docs of the strings.Compare doesn't mentioned the current implementation is slower than expected for some situations.

@go101
Copy link
Author

go101 commented Dec 16, 2021

One advantage of the strings.Compare way is the case order has much less impact on the performance, but the case order in the operator way impacts the performance much.

@go101
Copy link
Author

go101 commented Dec 17, 2021

I will close this issue, for I found a similar one: #31187

But I think close the issue for using strings.Compare is poor code or less-readable code is not acceptable.

@go101
Copy link
Author

go101 commented Dec 17, 2021

OK, it is closed already.

@go101
Copy link
Author

go101 commented Nov 1, 2022

Some notes:

Some conflict with the internal comment of strings.Compare: Basically no one should use strings.Compare.

BTW, there are more uses of this function out of switch blocks: https://sourcegraph.com/search?q=context:global+strings.Compare+lang:Go&patternType=standard

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants