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

proposal: standard library optimizations #39314

Closed
petar-dambovaliev opened this issue May 29, 2020 · 6 comments
Closed

proposal: standard library optimizations #39314

petar-dambovaliev opened this issue May 29, 2020 · 6 comments
Labels

Comments

@petar-dambovaliev
Copy link
Contributor

@petar-dambovaliev petar-dambovaliev commented May 29, 2020

I am going around the standard library and trying to find things to improve.
I don't know if this is a priority now but as you know people like to work on things they are interested in.
If you think changes like this will have a chance to get merged, i will gladly continue with this issue.

Example:

from:

func EqualIgnoreCaseOld(s1, s2 string) bool {
	if len(s1) != len(s2) {
		return false
	}
	for i := 0; i < len(s1); i++ {
		c1 := s1[i]
		if 'A' <= c1 && c1 <= 'Z' {
			c1 += 'a' - 'A'
		}
		c2 := s2[i]
		if 'A' <= c2 && c2 <= 'Z' {
			c2 += 'a' - 'A'
		}
		if c1 != c2 {
			return false
		}
	}
	return true
}

To:

func EqualIgnoreCase(s1, s2 string) bool {
	if len(s1) != len(s2) {
		return false
	}
	for i := 0; i < len(s1); i++ {
		if s1[i] &^ 'a' != s2[i] &^ 'a' {
			return false
		}
	}
	return true
}

These benchmarks are on random data.

BenchmarkEqualIgnoreCase-8 12356467 93.7 ns/op
BenchmarkEqualIgnoreCase1-8 11593638 99.6 ns/op
BenchmarkEqualIgnoreCaseOld-8 5494894 216 ns/op
BenchmarkEqualIgnoreCaseOld1-8 5293328 222 ns/op

@gopherbot gopherbot added this to the Proposal milestone May 29, 2020
@gopherbot gopherbot added the Proposal label May 29, 2020
@martisch
Copy link
Contributor

@martisch martisch commented May 29, 2020

Even while this is closed. A suggestion. Please use https://pkg.go.dev/golang.org/x/perf/cmd/benchstat?tab=doc and run multiple rounds (10 or 20) on a quiet machine to get output that looks like:

name        old time/op  new time/op  delta
GobEncode   13.6ms ± 1%  11.8ms ± 1%  -13.31% (p=0.016 n=4+5)
JSONEncode  32.1ms ± 1%  31.8ms ± 1%     ~    (p=0.286 n=4+5)

Its easier to judge the relative and consistency of the speedup that way.

@petar-dambovaliev
Copy link
Contributor Author

@petar-dambovaliev petar-dambovaliev commented May 29, 2020

@martisch so you don't want these types of contributions?

old.txt
goos: linux
goarch: amd64
pkg: awesomeProject4
BenchmarkEqualIgnoreCase-8 7785818 149 ns/op
BenchmarkEqualIgnoreCase-8 8085933 149 ns/op
BenchmarkEqualIgnoreCase-8 8055278 155 ns/op
BenchmarkEqualIgnoreCase-8 7693917 163 ns/op
BenchmarkEqualIgnoreCase-8 8016772 149 ns/op
PASS
ok awesomeProject4 6.842s

new.txt
goos: linux
goarch: amd64
pkg: awesomeProject4
BenchmarkEqualIgnoreCase-8 16568508 68.6 ns/op
BenchmarkEqualIgnoreCase-8 17050687 71.4 ns/op
BenchmarkEqualIgnoreCase-8 15403122 78.5 ns/op
BenchmarkEqualIgnoreCase-8 17192120 72.1 ns/op
BenchmarkEqualIgnoreCase-8 17139897 73.0 ns/op
PASS
ok awesomeProject4 6.426s

@randall77
Copy link
Contributor

@randall77 randall77 commented May 29, 2020

Contributions that improve the speed of the standard library are welcome.

There really isn't a need for a meta-issue like this, so it doesn't need to be open. Go ahead and submit specific optimization CLs when you have them. The contribution guide explains how to do it, if you haven't seen that already.

The optimization you propose for EqualIgnoreCase isn't correct. And EqualIgnoreCase doesn't appear anywhere in the stdlib. So I'm not sure what that example was supposed to be about...

@petar-dambovaliev
Copy link
Contributor Author

@petar-dambovaliev petar-dambovaliev commented May 29, 2020

@randall77 go/src/strconv/atof.go:17 func equalIgnoreCase(s1, s2 string) bool

@randall77
Copy link
Contributor

@randall77 randall77 commented May 29, 2020

That no longer exists in the stdlib at tip. As of https://go-review.googlesource.com/c/go/+/230737 , I think.

@martisch
Copy link
Contributor

@martisch martisch commented May 29, 2020

@martisch so you don't want these types of contributions?

Thats neither what I wrote nor my intention. I had assumed since you earlier closed the issue that this should have been stayed close as it was reopened without comment.

For discussions and questions about contributions its better to use https://groups.google.com/g/golang-dev.

Generally feel free to send performance improving CLs that show concretely where and how the code will be changed and the benchmark data.
https://golang.org/doc/contribute.html

No need to make it a proposal which is a more involved process of e.g. proposing a public API change for go. https://github.com/golang/proposal#readme

@martisch martisch reopened this May 29, 2020
@martisch martisch removed this from the Proposal milestone May 29, 2020
@martisch martisch removed the Proposal label May 29, 2020
@gopherbot gopherbot added the Proposal label May 29, 2020
@martisch martisch closed this May 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.