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

Improve memory performance of collector #331

Conversation

antoninbas
Copy link
Member

Avoid unnecessary slice creation when adding a record to a set. We implement this change for both data record and template records, but in practice most of the gains will come from data records.

Memory allocations (in bytes) are reduced by around 15% for BenchmarkCollector.

@antoninbas
Copy link
Member Author

Before change:

goos: darwin
goarch: amd64
pkg: github.com/vmware/go-ipfix/pkg/test
cpu: Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
BenchmarkCollector/ipv4-12         	     304	   3809314 ns/op	 1832212 B/op	   45001 allocs/op
BenchmarkCollector/ipv6-12         	     304	   3852429 ns/op	 1922795 B/op	   45000 allocs/op
PASS

After change:

goos: darwin
goarch: amd64
pkg: github.com/vmware/go-ipfix/pkg/test
cpu: Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
BenchmarkCollector/ipv4-12         	     306	   3785402 ns/op	 1512218 B/op	   44001 allocs/op
BenchmarkCollector/ipv6-12         	     310	   3828923 ns/op	 1602773 B/op	   44000 allocs/op
PASS

@antoninbas
Copy link
Member Author

@heanlan @dreamtalen let me know if you think this is not a correct change (functionality-wise)

@codecov
Copy link

codecov bot commented Oct 5, 2023

Codecov Report

Merging #331 (6d7c889) into main (c5e0992) will decrease coverage by 0.39%.
The diff coverage is 26.31%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #331      +/-   ##
==========================================
- Coverage   73.25%   72.87%   -0.39%     
==========================================
  Files          19       19              
  Lines        2808     2831      +23     
==========================================
+ Hits         2057     2063       +6     
- Misses        581      599      +18     
+ Partials      170      169       -1     
Flag Coverage Δ
integration-tests 51.53% <100.00%> (ø)
unit-tests 71.88% <26.31%> (-0.45%) ⬇️

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

Files Coverage Δ
pkg/collector/process.go 86.03% <100.00%> (ø)
pkg/entities/set.go 66.15% <60.00%> (+3.46%) ⬆️
pkg/entities/record.go 59.25% <13.33%> (-12.56%) ⬇️

... and 1 file with indirect coverage changes

Avoid unnecessary slice creation when adding a record to a set. We
implement this change for both data record and template records, but in
practice most of the gains will come from data records.

Memory allocations (in bytes) are reduced by around 15% for
BenchmarkCollector.

Signed-off-by: Antonin Bas <[email protected]>
@antoninbas antoninbas force-pushed the avoid-unnecessary-slice-creation-when-adding-record-to-set branch from b3d6e26 to 6d7c889 Compare October 5, 2023 23:03
Copy link
Contributor

@dreamtalen dreamtalen left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@heanlan heanlan left a comment

Choose a reason for hiding this comment

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

LGTM

Do we still need to keep the original AddRecord?

@antoninbas
Copy link
Member Author

LGTM

Do we still need to keep the original AddRecord?

Since it's a public API of the library, and there was no strong reason to remove it, I just kept it

@antoninbas antoninbas merged commit d9b91b5 into vmware:main Oct 10, 2023
7 of 9 checks passed
@antoninbas antoninbas deleted the avoid-unnecessary-slice-creation-when-adding-record-to-set branch October 10, 2023 22:14
heanlan pushed a commit to heanlan/go-ipfix that referenced this pull request Dec 7, 2023
Avoid unnecessary slice creation when adding a record to a set. We
implement this change for both data record and template records, but in
practice most of the gains will come from data records.

Memory allocations (in bytes) are reduced by around 15% for
BenchmarkCollector.

Signed-off-by: Antonin Bas <[email protected]>
heanlan pushed a commit that referenced this pull request Dec 7, 2023
Avoid unnecessary slice creation when adding a record to a set. We
implement this change for both data record and template records, but in
practice most of the gains will come from data records.

Memory allocations (in bytes) are reduced by around 15% for
BenchmarkCollector.

Signed-off-by: Antonin Bas <[email protected]>
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.

4 participants