replace strings.SplitN with strings.Cut #4405
Conversation
exec.go
Outdated
| u := strings.SplitN(user, ":", 2) | ||
| if len(u) > 1 { | ||
| gid, err := strconv.Atoi(u[1]) | ||
| uidString, gidString, doesHaveGid := strings.Cut(user, ":") | ||
| if doesHaveGid { | ||
| gid, err := strconv.Atoi(gidString) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("bad gid: %w", err) | ||
| } | ||
| p.User.GID = uint32(gid) | ||
| } | ||
| uid, err := strconv.Atoi(u[0]) | ||
| uid, err := strconv.Atoi(uidString) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("bad uid: %w", err) | ||
| } |
There was a problem hiding this comment.
It's common in go to use short variable names when the distance between defining and using them is short; also to avoid type suffixes (like String here`)
In situations like this, ok is also a common name (instead of the doesHaveGid); while doesHaveGid is somewhat more descriptive, it's likely clear from the code what it's handling; using ok can reduce cognitive overload, as a reader can recognize the pattern without understanding if doesHaveGid has additional purpose or (due to the longer name) possibly used further away from its declaration.
e.g., this could look like;
uid, gid, ok := strings.Cut(user, ":")
if ok {
v, err := strconv.Atoi(gid)
if err != nil {
return nil, fmt.Errorf("bad gid: %w", err)
}
p.User.GID = uint32(v)
}
v, err := strconv.Atoi(uid)
if err != nil {
return nil, fmt.Errorf("bad uid: %w", err)
}
p.User.UID = uint32(v)Somewhat orthogonal to this PR (as it's existing code), but wondering if the strconv.Atoi(gid) would be better replaced with strconv.ParseUint(gid, 10, 32), given that we're ultimately casting to a uint32; i.e. strconv.Atoi("4294967296") will result in gid (uint32) 0.
There was a problem hiding this comment.
thanks for your comment. I change the variable names.
There was a problem hiding this comment.
i did a little benchmark for the second issue you told. and the result is like this:
goos: linux
goarch: amd64
pkg: bench_go
cpu: Intel(R) Core(TM) i5-6300U CPU @ 2.40GHz
BenchmarkAtoiToUint32-4 116347470 10.19 ns/op
BenchmarkParseUint-4 62446058 17.87 ns/op
PASS
ok bench_go 3.086s
as you can see using strconv.Atoi has better performance than ParseUint.
and also this is the code used for benchmark
package main
import (
"strconv"
"testing"
)
func AtoiToUint32(s string) uint32 {
i, _ := strconv.Atoi(s)
return uint32(i)
}
func ParseUint(s string) uint32 {
i, _ := strconv.ParseUint(s, 10, 32)
return uint32(i)
}
func BenchmarkAtoiToUint32(b *testing.B) {
for i := 0; i < b.N; i++ {
_ = AtoiToUint32("123456")
}
}
func BenchmarkParseUint(b *testing.B) {
for i := 0; i < b.N; i++ {
_ = ParseUint("123456")
}
}now with these results, do you think it's better if we change ParseUint to Atoi? and also i didn't know that but ParseUint return an uint64 so we need uint32 casting anyway.
There was a problem hiding this comment.
Thanks for checking that. Hm.. that's a good point. I was hoping it wouldn't make a big difference. It looks like we use the strconv.ParseUint approach in some other places already, so it's not unprecedented, and we're still looking at nanoseconds difference, so it's not a massive overhead, but of course every bit stacks up.
I was mostly looking at the "correctness" part here, because invalid values could be silently ignored, which may not be desirable, so perhaps worth the trade-off.
Alternatively, we could manually check for values to be out of range
@kolyshkin any thoughts?
☝️ regardless, I think it's a change we should do in a separate PR (for visibility; to not stuff it into a refactor).
done |
|
oh! If you still have time; I notice your DCO sign-off is using your GitHub handle, not your actual name; https://github.com/opencontainers/runc/blob/main/CONTRIBUTING.md#sign-your-work
To be on the safe side (the DCO bot is sometimes picky), you may want to update your Git config ( |
thanks for saying. I changed my user.name as well as my sign |
|
in the test case. it gives permission denied. can you guide me, what is the problem. I didn't change anything that need permission to run |
|
I think it may have been a flakey test; it looks green now. I see there's a merge commit in the PR though; could you try doing another rebase to get rid of the merge commit? After that changes LGTM |
amghazanfari
force-pushed
the
main
branch
2 times, most recently
from
74afb67
to
ee73a09
Compare
|
And, of course now it's failing again! :sadpanda: This one looks very unrelated, but for some reason it wants to use Same for this one; this one is missing |
|
Oh! Perhaps related to this PR? |
|
@kolyshkin Looks like DCO is now picky again 😞 perhaps needs a 
|
|
Maybe related to the commit msg: @amghazanfari Please remove the first line. |
sorry my bad. i fixed it |
Signed-off-by: Amir M. Ghazanfari <[email protected]>
there are some cases in the code that this pattern used "strings.SplitN(expression, pattern, 2)"
as we see in #4403 strings.Cut has better performance than strings.SplitN.
i changed some of these cases (for rest of them i wasn't sure that it break the functionality or not)