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

tune slice append performance #850

Merged
merged 4 commits into from
Mar 5, 2024
Merged

Conversation

hjweddie
Copy link
Contributor

I find some codes abount binlog, which is doing slices assignment or append could be simply changed, so that we can get some perfomance tuning.
Hope that it could help

some corresponding benchmark code may be as follow:

package main_test

import (
	"testing"
)

func BenchmarkAppend(b *testing.B) {
	strs := []string{"a", "b", "c"}
	tmp := make([]string, 0, len(strs))

	b.ResetTimer()
	for i := 0; i < b.N; i++ {
		for _, str := range strs {
			tmp = append(tmp, str)
		}
	}
}

func BenchmarkAssign(b *testing.B) {
	strs := []string{"a", "b", "c"}
	tmp := make([]string, len(strs))

	b.ResetTimer()
	for i := 0; i < b.N; i++ {
		for i, str := range strs {
			tmp[i] = str
		}
	}
}

@hjweddie
Copy link
Contributor Author

umm, I will check ci failure result

@lance6716
Copy link
Collaborator

func BenchmarkAppend(b *testing.B) {
	strs := []string{"a", "b", "c"}
	tmp := make([]string, 0, len(strs))

	b.ResetTimer()
	for i := 0; i < b.N; i++ {
		for _, str := range strs {
			tmp = append(tmp, str)
		}
	}
}

for b.N larger than 1, your code will let tmp grow so it's slower.

@hjweddie
Copy link
Contributor Author

hjweddie commented Mar 1, 2024

func BenchmarkAppend(b *testing.B) {
	strs := []string{"a", "b", "c"}
	tmp := make([]string, 0, len(strs))

	b.ResetTimer()
	for i := 0; i < b.N; i++ {
		for _, str := range strs {
			tmp = append(tmp, str)
		}
	}
}

for b.N larger than 1, your code will let tmp grow so it's slower.

Agree it! I intend to reduce the overhead of slice growth

@hjweddie
Copy link
Contributor Author

hjweddie commented Mar 1, 2024

ci jobs pass

@lance6716
Copy link
Collaborator

I mean your benchmark is not correct

func BenchmarkAppend(b *testing.B) {
	strs := []string{"a", "b", "c"}
	tmp := make([]string, 0, len(strs))

	b.ResetTimer()
	for i := 0; i < b.N; i++ {
+		tmp = tmp[:0]
		for _, str := range strs {
			tmp = append(tmp, str)
		}
	}
}
goos: darwin
goarch: arm64
pkg: awesomeProject
BenchmarkAppend
BenchmarkAppend-8   	373262626	         3.161 ns/op
BenchmarkAssign
BenchmarkAssign-8   	541436124	         2.219 ns/op
PASS

@hjweddie
Copy link
Contributor Author

hjweddie commented Mar 2, 2024

package main_test

import (
"testing"
)

func BenchmarkAppend(b *testing.B) {
strs := []string{"a", "b", "c"}
tmp := make([]string, 0, len(strs))

b.ResetTimer()
for i := 0; i < b.N; i++ {
for _, str := range strs {
tmp = append(tmp, str)
}
}
}

func BenchmarkAssign(b *testing.B) {
strs := []string{"a", "b", "c"}
tmp := make([]string, len(strs))

b.ResetTimer()
for i := 0; i < b.N; i++ {
for i, str := range strs {
tmp[i] = str
}
}
}

You are right. I put slice intialization in the loop and find slice assignment gets just a little more performance than append.

Because function parseSingleEvent could be called with high frequency in binlog parsing, I think it may be acceptable

func BenchmarkAppend(b *testing.B) {
	strs := []string{"a", "b", "c"}

	b.ResetTimer()
	for i := 0; i < b.N; i++ {
		tmp := make([]string, 0, len(strs))
		for _, str := range strs {
			tmp = append(tmp, str)
		}
	}
}

func BenchmarkAssign(b *testing.B) {
	strs := []string{"a", "b", "c"}

	b.ResetTimer()
	for i := 0; i < b.N; i++ {
		tmp := make([]string, len(strs))
		for i, str := range strs {
			tmp[i] = str
		}
	}
}
goos: darwin
goarch: arm64
pkg: test
BenchmarkAppend-8   	29462312	        40.81 ns/op
BenchmarkAssign-8   	30202609	        39.69 ns/op
PASS
ok  	test	2.600s

@hjweddie
Copy link
Contributor Author

hjweddie commented Mar 2, 2024

If I try to make strs be a slice with a larger size, performance difference between assign and append will be larger too

func BenchmarkAppend(b *testing.B) {
	strs := make([]string, 0)
	for i := 0; i < 10000; i++ {
		strs = append(strs, "d")
	}

	b.ResetTimer()
	for i := 0; i < b.N; i++ {
		tmp := make([]string, 0, len(strs))
		for _, str := range strs {
			tmp = append(tmp, str)
		}
	}
}

func BenchmarkAssign(b *testing.B) {
	strs := make([]string, 0)
	for i := 0; i < 10000; i++ {
		strs = append(strs, "d")
	}

	b.ResetTimer()
	for i := 0; i < b.N; i++ {
		tmp := make([]string, len(strs))
		for i, str := range strs {
			tmp[i] = str
		}
	}
}
goos: darwin
goarch: arm64
pkg: test
BenchmarkAppend-8   	   42304	     31042 ns/op
BenchmarkAssign-8   	   37588	     29779 ns/op
PASS
ok  	test	3.158s

Copy link
Collaborator

@lance6716 lance6716 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rest lgtm

replication/binlogstreamer.go Outdated Show resolved Hide resolved
replication/event.go Outdated Show resolved Hide resolved
replication/event.go Outdated Show resolved Hide resolved
replication/parser.go Outdated Show resolved Hide resolved
@hjweddie
Copy link
Contributor Author

hjweddie commented Mar 4, 2024

I will check it later

@hjweddie
Copy link
Contributor Author

hjweddie commented Mar 4, 2024

I have changed the code as suggestion

@lance6716 lance6716 merged commit 4187985 into go-mysql-org:master Mar 5, 2024
13 checks passed
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.

2 participants