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

Use strings.Cut() instead of string.SplitN() #4049

Merged
merged 1 commit into from
May 17, 2023

Conversation

ash2k
Copy link
Contributor

@ash2k ash2k commented May 3, 2023

  1. strings.Cut() generates less garbage as it does not allocate the slice to hold parts.
  2. Code with strings.Cut() is more readable as it's easier to deal with bool vs int - less branching is usually required, fewer arguments to the function call, simpler semantics. See bytes, strings: add Cut golang/go#46336.
func BenchmarkCut(b *testing.B) {
	const s = "key=val"
	const sep = "="

	var splitSink []string
	var cut1, cut2 string
	var found bool

	b.Run("split", func(b *testing.B) {
		b.ReportAllocs()
		b.ResetTimer()
		for i := 0; i < b.N; i++ {
			splitSink = strings.SplitN(s, sep, 2)
		}
	})
	b.Run("cut", func(b *testing.B) {
		b.ReportAllocs()
		b.ResetTimer()
		for i := 0; i < b.N; i++ {
			cut1, cut2, found = strings.Cut(s, sep)
		}
	})
	_ = splitSink
	_ = cut1
	_ = cut2
	_ = found
}
goos: darwin
goarch: arm64
pkg: go.opentelemetry.io/otel/sdk/trace
BenchmarkCut
BenchmarkCut/split
BenchmarkCut/split-10         	38834322	        28.62 ns/op	      32 B/op	       1 allocs/op
BenchmarkCut/cut
BenchmarkCut/cut-10           	215805100	         5.536 ns/op	       0 B/op	       0 allocs/op
PASS

@ash2k ash2k changed the title Use strings.Cut instead of string.SplitN Use strings.Cut() instead of string.SplitN() May 3, 2023
@codecov
Copy link

codecov bot commented May 3, 2023

Codecov Report

Merging #4049 (eee2369) into main (8445f21) will increase coverage by 0.0%.
The diff coverage is 93.1%.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #4049   +/-   ##
=====================================
  Coverage   83.3%   83.3%           
=====================================
  Files        181     181           
  Lines      13934   13925    -9     
=====================================
- Hits       11614   11610    -4     
+ Misses      2099    2094    -5     
  Partials     221     221           
Impacted Files Coverage Δ
baggage/baggage.go 99.0% <83.3%> (+1.5%) ⬆️
exporters/otlp/internal/envconfig/envconfig.go 82.8% <100.0%> (ø)
internal/internaltest/text_map_propagator.go 81.8% <100.0%> (ø)
sdk/resource/env.go 94.0% <100.0%> (ø)
sdk/resource/os_release_unix.go 83.0% <100.0%> (ø)
semconv/internal/http.go 100.0% <100.0%> (ø)

... and 2 files with indirect coverage changes

@ash2k ash2k force-pushed the ash2k/strings-cut branch 2 times, most recently from f323f59 to 3e80b34 Compare May 3, 2023 02:56
@MrAlias
Copy link
Contributor

MrAlias commented May 3, 2023

Please include some sort of performance benchmarking with this PR to motivate these changes.

@ash2k
Copy link
Contributor Author

ash2k commented May 4, 2023

@MrAlias Added benchmark to the description.

@ash2k ash2k force-pushed the ash2k/strings-cut branch from 3e80b34 to b1bd42b Compare May 8, 2023 00:28
baggage/baggage.go Outdated Show resolved Hide resolved
sdk/resource/env.go Outdated Show resolved Hide resolved
sdk/resource/os_release_unix.go Outdated Show resolved Hide resolved
@ash2k ash2k force-pushed the ash2k/strings-cut branch from b1bd42b to e778c34 Compare May 8, 2023 10:20
@ash2k
Copy link
Contributor Author

ash2k commented May 8, 2023

@pellared Thanks. I was not sure how to name those and hoped to get some suggestions during the review.

@pellared
Copy link
Member

pellared commented May 8, 2023

@pellared Thanks. I was not sure how to name those and hoped to get some suggestions during the review.

👍 Just please add "regular commits" (do not rebase) in future as it makes reviewing diffs a lot easier 😉

strings.Cut() generates less garbage as it does not allocate the slice to hold parts.
@ash2k ash2k force-pushed the ash2k/strings-cut branch from e778c34 to eee2369 Compare May 17, 2023 10:36
@ash2k
Copy link
Contributor Author

ash2k commented May 17, 2023

Fixed the conflict. It's ready for merging.

@MrAlias
Copy link
Contributor

MrAlias commented May 17, 2023

We still need another review from a person with a different employer.

@MrAlias MrAlias merged commit f95bee2 into open-telemetry:main May 17, 2023
@ash2k ash2k deleted the ash2k/strings-cut branch May 17, 2023 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants