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

perf: avoid allocations with (*regexp.Regexp).MatchString #3881

Merged
merged 2 commits into from
Nov 15, 2023
Merged

perf: avoid allocations with (*regexp.Regexp).MatchString #3881

merged 2 commits into from
Nov 15, 2023

Conversation

Juneezee
Copy link
Contributor

what

Replace (*regexp.Regexp).Match([]byte(...)) with (*regexp.Regexp).MatchString

why

We should use (*regexp.Regexp).MatchString instead of (*regexp.Regexp).Match([]byte(...)) when matching string to avoid unnecessary []byte conversions and reduce allocations. A one-line change for free performance gain.

Example benchmark:

func BenchmarkMatch(b *testing.B) {
	for i := 0; i < b.N; i++ {
		if match := branchRegex.Match([]byte("main")); !match {
			b.Fail()
		}
	}
}

func BenchmarkMatchString(b *testing.B) {
	for i := 0; i < b.N; i++ {
		if match := branchRegex.MatchString("main"); !match {
			b.Fail()
		}
	}
}
goos: linux
goarch: amd64
pkg: github.com/runatlantis/atlantis/server/core/config cpu: AMD Ryzen 7 PRO 4750U with Radeon Graphics
BenchmarkMatch-16          	 8269699	       141.4 ns/op	       4 B/op	       1 allocs/op
BenchmarkMatchString-16    	14298446	        95.81 ns/op	       0 B/op	       0 allocs/op
PASS
ok  	github.com/runatlantis/atlantis/server/core/config	2.784s

tests

go test ./server/core/config
ok  	github.com/runatlantis/atlantis/server/core/config	0.024s

references

We should use `(*regexp.Regexp).MatchString` instead of
`(*regexp.Regexp).Match([]byte(...))` when matching string to avoid
unnecessary `[]byte` conversions and reduce allocations.

Example benchmark:

func BenchmarkMatch(b *testing.B) {
	for i := 0; i < b.N; i++ {
		if match := branchRegex.Match([]byte("main")); !match {
			b.Fail()
		}
	}
}

func BenchmarkMatchString(b *testing.B) {
	for i := 0; i < b.N; i++ {
		if match := branchRegex.MatchString("main"); !match {
			b.Fail()
		}
	}
}

goos: linux
goarch: amd64
pkg: github.com/runatlantis/atlantis/server/core/config
cpu: AMD Ryzen 7 PRO 4750U with Radeon Graphics
BenchmarkMatch-16          	 8269699	       141.4 ns/op	       4 B/op	       1 allocs/op
BenchmarkMatchString-16    	14298446	        95.81 ns/op	       0 B/op	       0 allocs/op
PASS
ok  	github.com/runatlantis/atlantis/server/core/config	2.784s

Signed-off-by: Eng Zer Jun <[email protected]>
@Juneezee Juneezee requested a review from a team as a code owner October 20, 2023 18:19
@github-actions github-actions bot added the go Pull requests that update Go code label Oct 20, 2023
@jamengual
Copy link
Contributor

@GenPage

@jamengual jamengual added the waiting-on-review Waiting for a review from a maintainer label Oct 20, 2023
@nitrocode
Copy link
Member

Fwiw I agree with this change.

I see some references online that also point to a performance gain

They are very similar. while slices like []byte are mutable (you can change values in the underlying array) strings are immutable.
Internally []byte consists of a length, capacity (both int values) and a pointer to the first element [1]. Strings only consist of the length and the pointer because capacity and length are always the same since strings are immutable.

What are the dangers of casting between them?

Wasted performance and memory

Why are regexp functions even relevant for []byte?

Because it is cheap. The function can easily return a sub-slice without copying any data.

https://groups.google.com/g/golang-nuts/c/YmiYPe7lako?pli=1

@jamengual jamengual added this to the v0.26.0 milestone Nov 15, 2023
@jamengual
Copy link
Contributor

/cherry-pick release-0.26

@jamengual jamengual merged commit fd600d3 into runatlantis:main Nov 15, 2023
24 checks passed
@jamengual
Copy link
Contributor

@Juneezee Thanks for the contribution

ijames-gc pushed a commit to gocardless/atlantis that referenced this pull request Feb 13, 2024
…tis#3881)

We should use `(*regexp.Regexp).MatchString` instead of
`(*regexp.Regexp).Match([]byte(...))` when matching string to avoid
unnecessary `[]byte` conversions and reduce allocations.

Example benchmark:

func BenchmarkMatch(b *testing.B) {
	for i := 0; i < b.N; i++ {
		if match := branchRegex.Match([]byte("main")); !match {
			b.Fail()
		}
	}
}

func BenchmarkMatchString(b *testing.B) {
	for i := 0; i < b.N; i++ {
		if match := branchRegex.MatchString("main"); !match {
			b.Fail()
		}
	}
}

goos: linux
goarch: amd64
pkg: github.com/runatlantis/atlantis/server/core/config
cpu: AMD Ryzen 7 PRO 4750U with Radeon Graphics
BenchmarkMatch-16          	 8269699	       141.4 ns/op	       4 B/op	       1 allocs/op
BenchmarkMatchString-16    	14298446	        95.81 ns/op	       0 B/op	       0 allocs/op
PASS
ok  	github.com/runatlantis/atlantis/server/core/config	2.784s

Signed-off-by: Eng Zer Jun <[email protected]>
Co-authored-by: PePe Amengual <[email protected]>
ijames-gc pushed a commit to gocardless/atlantis that referenced this pull request Feb 13, 2024
…tis#3881)

We should use `(*regexp.Regexp).MatchString` instead of
`(*regexp.Regexp).Match([]byte(...))` when matching string to avoid
unnecessary `[]byte` conversions and reduce allocations.

Example benchmark:

func BenchmarkMatch(b *testing.B) {
	for i := 0; i < b.N; i++ {
		if match := branchRegex.Match([]byte("main")); !match {
			b.Fail()
		}
	}
}

func BenchmarkMatchString(b *testing.B) {
	for i := 0; i < b.N; i++ {
		if match := branchRegex.MatchString("main"); !match {
			b.Fail()
		}
	}
}

goos: linux
goarch: amd64
pkg: github.com/runatlantis/atlantis/server/core/config
cpu: AMD Ryzen 7 PRO 4750U with Radeon Graphics
BenchmarkMatch-16          	 8269699	       141.4 ns/op	       4 B/op	       1 allocs/op
BenchmarkMatchString-16    	14298446	        95.81 ns/op	       0 B/op	       0 allocs/op
PASS
ok  	github.com/runatlantis/atlantis/server/core/config	2.784s

Signed-off-by: Eng Zer Jun <[email protected]>
Co-authored-by: PePe Amengual <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
go Pull requests that update Go code waiting-on-review Waiting for a review from a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants