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 fix for encode and decode to use bytes data type #207

Merged
merged 1 commit into from
Jun 1, 2021

Conversation

srikartati
Copy link
Contributor

@srikartati srikartati commented May 29, 2021

As part of Antrea benchmarking, we found that bytes.Buffer type is incurring very high overhead in terms of allocations and memory consumed. Moving to bytes data type instead of bytes.Buffer wrapper.

Fixes: #205

Before fix:

vagrantÉk8s-node-worker-1:ü/antrea/pkg/flowaggregator$ go test -test.v -run=none -test.benchmem  -bench=. -memprofile memprofile.out -cpuprofile profile.out
goos: linux
goarch: amd64
pkg: antrea.io/antrea/pkg/flowaggregator
cpu: Intel(R) Core(TM) i9-9980HK CPU É 2.40GHz
BenchmarkIntraNodeFlowRecords
    flowaggregator_perf_test.go:446: Num messages received: 1516755
BenchmarkIntraNodeFlowRecords-2   	       1	120001712897 ns/op	22902440680 B/op	431854675 allocs/op
PASS
ok  	antrea.io/antrea/pkg/flowaggregator	120.119s

After this fix:

vagrant@k8s-node-worker-1:~/antrea/pkg/flowaggregator$ go test -test.v -run=none -test.benchmem  -bench=. -memprofile memprofile.out -cpuprofile profile.out
goos: linux
goarch: amd64
pkg: antrea.io/antrea/pkg/flowaggregator
cpu: Intel(R) Core(TM) i9-9980HK CPU @ 2.40GHz
BenchmarkIntraNodeFlowRecords
    flowaggregator_perf_test.go:446: Num messages received: 1526508
BenchmarkIntraNodeFlowRecords-2   	       1	120005595596 ns/op	19760001232 B/op	356061206 allocs/op
PASS
ok  	antrea.io/antrea/pkg/flowaggregator	120.328s

@codecov
Copy link

codecov bot commented May 29, 2021

Codecov Report

Merging #207 (72263b4) into main (6d5fafa) will increase coverage by 0.57%.
The diff coverage is 65.21%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #207      +/-   ##
==========================================
+ Coverage   78.79%   79.36%   +0.57%     
==========================================
  Files          17       17              
  Lines        2155     2118      -37     
==========================================
- Hits         1698     1681      -17     
+ Misses        307      300       -7     
+ Partials      150      137      -13     
Flag Coverage Δ
integration-tests 56.95% <52.17%> (-0.56%) ⬇️
unit-tests 79.03% <65.21%> (+0.57%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/entities/set.go 70.58% <50.00%> (-6.01%) ⬇️
pkg/exporter/process.go 65.86% <50.00%> (-1.00%) ⬇️
pkg/collector/process.go 87.20% <54.54%> (-2.27%) ⬇️
pkg/entities/ie.go 55.17% <64.93%> (+0.91%) ⬆️
pkg/entities/record.go 76.71% <70.00%> (+7.48%) ⬆️
pkg/intermediate/aggregate.go 73.54% <100.00%> (+0.40%) ⬆️

Copy link
Contributor

@zyiou zyiou left a comment

Choose a reason for hiding this comment

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

Overall LGTM. Thanks for the changes. Encoding, decoding function and the tests looks much cleaner.

@srikartati
Copy link
Contributor Author

Overall LGTM. Thanks for the changes. Encoding, decoding function and the tests looks much cleaner.

Thanks for the review, Yiou.
I think we can optimize further by doing pre-allocation of bytes slice in the record by using total elements size. This will improve the performance further. Will take it up in future PR.

@srikartati srikartati merged commit 4fafcfb into vmware:main Jun 1, 2021
@srikartati srikartati deleted the encode_fix branch June 1, 2021 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Performance issue: EncodeToIEDataType takes up lot of CPU cycles and memory
3 participants