strings: refactor logic ToLower

[ghost]

hi everyone, I was thinking about the method ToLower in standard library strings then I saw as the pkg Redis implements this idea. Then I identified that is very better than the standard library. Maybe we can improve this package strings.

Let's go to talk about this?

More details below:

e.g

func isLower(s string) bool {
	for i := 0; i < len(s); i++ {
		c := s[i]
		if c >= 'A' && c <= 'Z' {
			return false
		}
	}
	return true
}

func ToLower(s string) string {
	if isLower(s) {
		return s
	}

	b := make([]byte, len(s))
	for i := range b {
		c := s[i]
		if c >= 'A' && c <= 'Z' {
			c += 'a' - 'A'
		}
		b[i] = c
	}
	return BytesToString(b)
}

func BytesToString(b []byte) string {
	return *(*string)(unsafe.Pointer(&b))
}

reference: https://github.com/go-redis/redis/blob/6e4eb2e3acad9575fb1e2c8690a3c1decf2e59e5/internal/util.go#L22

One little detail I believe that we cannot use unsafe, I would use just string(bytes)

//BenchmarkTestToLowerStdLib           4497225               265.1 ns/op            64 B/op          1 allocs/op
func BenchmarkTestToLowerStdLib(b *testing.B) {
	for i := 0; i < b.N; i++ {
		_ = strings.ToLower(`RENAN BASTOS 93 AOSDAJDJAIDJAIDAJIaidsjjaidijadsjiadjiOOKKO`)
	}
}

//BenchmarkTestToLowerInternalRedis    15342235                77.65 ns/op           64 B/op          1 allocs/op
func BenchmarkTestToLowerInternalRedis(b *testing.B) {
	for i := 0; i < b.N; i++ {
		_ = ToLower(`RENAN BASTOS 93 AOSDAJDJAIDJAIDAJIaidsjjaidijadsjiadjiOOKKO`)
	}
}

I am sorry for anything but I want to help our loved Go.

[ghost]

I opened this issue using my work account I will follow discuss using @renanbastos93

[ianlancetaylor]

There is no API change here so taking it out of the proposal process.

We don't want to to use unsafe here. We also don't want to use a string conversion, as that will add an allocation. The current code already uses strings.Builder, which effectively handles the unsafe code in a safe way.

Note that your replacement code does not correctly handle UTF-8 encodings that are not ASCII.

It's worth investigating why the current code is slower than your version, and see if we can fix that without introducing unsafe. Perhaps it is the fact that WriteByte calls copyCheck? But I don't really know.

[renanbastos93]

@ianlancetaylor I got it, also I want not to use it unsafely. Thanks for your fast answer.

About WriteByte calls copyCheck: I don't know too. How can I help you?

[renanbastos93]

I saw code from copyCheck it using unsafe o.O

func (b *Builder) copyCheck() {
	if b.addr == nil {
		// This hack works around a failing of Go's escape analysis
		// that was causing b to escape and be heap allocated.
		// See issue 23382.
		// TODO: once issue 7921 is fixed, this should be reverted to
		// just "b.addr = b".
		b.addr = (*Builder)(noescape(unsafe.Pointer(b)))
	} else if b.addr != b {
		panic("strings: illegal use of non-zero Builder copied by value")
	}
}

The WriteByte uses it

func (b *Builder) WriteByte(c byte) error {
	b.copyCheck()
	b.buf = append(b.buf, c)
	return nil
}

Maybe we should review this approach.

[ianlancetaylor]

We decided to permit strings.Builder to use unsafe to minimize allocations, but we don't want it to leak out to the rest of the strings package.

[renanbastos93]

Alright, I understood.
I was trying running unit tests local but it raises not allow that we use the package internal. =/

I am analyzing code ToLower, it uses Grow too. This method Grow uses a method named copy.

func (b *Builder) grow(n int) {
	buf := make([]byte, len(b.buf), 2*cap(b.buf)+n)
	copy(buf, b.buf)
	b.buf = buf
}

I am trying to understand all logic to help us.

[elagergren-spideroak]

I was trying running unit tests local but it raises not allow that we use the package internal. =/

@renanbastos93 clone the Go repo, build the compiler with make.bash, then use that compiler to run the strings unit tests.

[renanbastos93]

I was trying running unit tests local but it raises not allow that we use the package internal. =/

@renanbastos93 clone the Go repo, build the compiler with make.bash, then use that compiler to run the strings unit tests.

I tried many times but keep to the problem.

 strings/strings.go:14:2: use of internal package internal/bytealg not allowed

@elagergren-spideroak
Can you help me?

[renanbastos93]

I was trying running unit tests local but it raises not allow that we use the package internal. =/

@renanbastos93 clone the Go repo, build the compiler with make.bash, then use that compiler to run the strings unit tests.

I tried many times but keep to the problem.

 strings/strings.go:14:2: use of internal package internal/bytealg not allowed

@elagergren-spideroak Can you help me?

I got run unit tests now I am try run bench tests.

[renanbastos93]

Did someone get to discover something?

[gopherbot]

Change https://go.dev/cl/423874 mentions this issue: strings: roll back string.Builder to get better performances from ToUpper()/ToLower() with long string

[renanbastos93]

@gopherbot Awesome, but I don't see an open PR. I saw this link, it is an excellent approach. Maybe we can close to this issue, right?

[renanbastos93]

@panjf2000 Do you need any help?

[panjf2000]

@gopherbot Awesome, but I don't see an open PR. I saw this link, it is an excellent approach. Maybe we can close to this issue, right?

In fact, Github repo of Go is just a mirror repo, the actual Git repo of Go is located at https://go.googlesource.com/go, described in https://github.com/golang/go/blob/master/README.md.

Thus, Go team and contributors work in https://go-review.googlesource.com, managing the source code of Go and reviewing PR/CL over there.

@renanbastos93

[panjf2000]

@panjf2000 Do you need any help?

I've tried several ways to optimize strings.ToLower()/ToUpper(), but it seemed to be unlikely to achieve the approximate improvement as BenchmarkTestToLowerInternalRedis you provided without introducing unsafe.

[renanbastos93]

@panjf2000 Do you need any help?

I've tried several ways to optimize strings.ToLower()/ToUpper(), but it seemed to be unlikely to achieve the approximate improvement as BenchmarkTestToLowerInternalRedis you provided without introducing unsafe.

@panjf2000 Yeah, I do agree with you dude we need to find a way simple to resolve it safely. Even so, tried many ways too but did not succeed. Maybe, we can think together. We go to create a list of references that help we want to resolve it.

1. Greater performance
2. Safely
3. Using Builder or not
4. Some algorithm to this (exists?)

@gopherbot Awesome, but I don't see an open PR. I saw this link, it is an excellent approach. Maybe we can close to this issue, right?

In fact, Github repo of Go is just a mirror repo, the actual Git repo of Go is located at https://go.googlesource.com/go, described in https://github.com/golang/go/blob/master/README.md.

Thus, Go team and contributors work in https://go-review.googlesource.com, managing the source code of Go and reviewing PR/CL over there.

@renanbastos93

Thanks for explaining to me

[gopherbot]

Change https://go.dev/cl/424139 mentions this issue: strings: speed up ToUpper()/ToLower() by batch writing data with Builder