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

Reduce memory allocations in interpolation #5909

Merged
merged 1 commit into from
Sep 6, 2021

Conversation

philippgille
Copy link
Contributor

@philippgille philippgille commented Aug 26, 2021

When using venom I found the performance to be not as good as I hoped and used pprof to try to find some places where it can be improved. Venom seems to use interpolate.Do a lot, so I looked if there's anything to improve and found something that seems safe and doesn't make the code more complex:

Instead of allocating new maps, the existing ones can be reused. Deleting the keys while ranging over a map is safe in Go, see the section in Effective Go:

for key := range m {
    if key.expired() {
        delete(m, key)
    }
}

Previous benchmark:

$ go test -run=XXX -bench=. -benchmem
goos: darwin
goarch: amd64
pkg: github.com/ovh/cds/sdk/interpolate
cpu: Intel(R) Core(TM) i7-8569U CPU @ 2.80GHz
BenchmarkDo-8          	    6561	    154820 ns/op	  115794 B/op	    1176 allocs/op
BenchmarkDoNothing-8   	   68073	     18401 ns/op	   10458 B/op	     119 allocs/op
PASS
ok  	github.com/ovh/cds/sdk/interpolate	2.824s
$ go test -run=XXX -bench=. -benchmem
goos: darwin
goarch: amd64
pkg: github.com/ovh/cds/sdk/interpolate
cpu: Intel(R) Core(TM) i7-8569U CPU @ 2.80GHz
BenchmarkDo-8          	    7088	    149979 ns/op	  115689 B/op	    1176 allocs/op
BenchmarkDoNothing-8   	   66132	     18032 ns/op	   10456 B/op	     119 allocs/op
PASS
ok  	github.com/ovh/cds/sdk/interpolate	2.582s

After the change:

$ go test -run=XXX -bench=. -benchmem
goos: darwin
goarch: amd64
pkg: github.com/ovh/cds/sdk/interpolate
cpu: Intel(R) Core(TM) i7-8569U CPU @ 2.80GHz
BenchmarkDo-8          	    7345	    141649 ns/op	   96200 B/op	    1161 allocs/op
BenchmarkDoNothing-8   	   64874	     18045 ns/op	   10456 B/op	     119 allocs/op
PASS
ok  	github.com/ovh/cds/sdk/interpolate	2.614s
$ go test -run=XXX -bench=. -benchmem
goos: darwin
goarch: amd64
pkg: github.com/ovh/cds/sdk/interpolate
cpu: Intel(R) Core(TM) i7-8569U CPU @ 2.80GHz
BenchmarkDo-8          	    7208	    141367 ns/op	   96059 B/op	    1161 allocs/op
BenchmarkDoNothing-8   	   65907	     17964 ns/op	   10458 B/op	     119 allocs/op
PASS
ok  	github.com/ovh/cds/sdk/interpolate	2.527s

I haven't measured the difference when running a Venom scenario and it's probably really small, but I thought it's worth improving anyway.

@ovh/cds

@philippgille philippgille force-pushed the patch-1 branch 2 times, most recently from 2e47524 to c6cfed9 Compare August 26, 2021 23:04
Instead of allocating new maps, the existing ones can be reused

Signed-off-by: Philipp Gillé <[email protected]>
@richardlt richardlt merged commit c80557c into ovh:master Sep 6, 2021
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