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

Add new Go Benchmarks #329

Merged
merged 2 commits into from
Oct 4, 2023
Merged

Add new Go Benchmarks #329

merged 2 commits into from
Oct 4, 2023

Conversation

antoninbas
Copy link
Member

A few new benchmarks, which can be useful when validating improvements to the implementation.

A few new benchmarks, which can be useful when validating improvements
to the implementation.

Signed-off-by: Antonin Bas <[email protected]>
@codecov
Copy link

codecov bot commented Oct 3, 2023

Codecov Report

Merging #329 (58dd8ff) into main (614e68c) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #329   +/-   ##
=======================================
  Coverage   73.67%   73.67%           
=======================================
  Files          19       19           
  Lines        2800     2800           
=======================================
  Hits         2063     2063           
  Misses        572      572           
  Partials      165      165           
Flag Coverage Δ
integration-tests 53.82% <ø> (ø)
unit-tests 72.53% <ø> (ø)

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

dreamtalen
dreamtalen previously approved these changes Oct 4, 2023
Copy link

@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.
While trying this benchmark locally, I found the IPv4 AddRecord and AggregateMsg benchmarks are approximately 20% slower than the IPv6 one.
Have you also seen similar results Antonin?

BenchmarkAddRecordIPv4Addresses-10                     	  335524	      3345 ns/op
BenchmarkAddRecordIPv6Addresses-10                     	  404529	      2855 ns/op
BenchmarkAggregateMsgByFlowKey/ipv4-10         	   58531	     20586 ns/op
BenchmarkAggregateMsgByFlowKey/ipv6-10         	   82375	     15100 ns/op

@antoninbas
Copy link
Member Author

@dreamtalen For BenchmarkAddRecord, I have also observed that. It's because the IPv4 benchmarks is not the same as the IPv6 one (more checks). Although I will update it so that they do the same. It doesn't really make sense to have them be different.

For BenchmarkAggregateMsgByFlowKey, the IPv4 one is faster for me, as expected.

BenchmarkAggregateMsgByFlowKey/ipv4-12         	  181294	      6341 ns/op	     696 B/op	      12 allocs/op
BenchmarkAggregateMsgByFlowKey/ipv6-12         	  164910	      7327 ns/op	     792 B/op	      13 allocs/op

@antoninbas
Copy link
Member Author

@dreamtalen I unified the test code, PTAL

For BenchmarkAggregateMsgByFlowKey, you could give it another try (I recommend adding -test.benchmem as well). Your computer may have been running something else at the time.

Copy link

@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

@dreamtalen
Copy link

@dreamtalen I unified the test code, PTAL

For BenchmarkAggregateMsgByFlowKey, you could give it another try (I recommend adding -test.benchmem as well). Your computer may have been running something else at the time.

Thank you. As expected, the IPv4 one now performs slightly better in both benchmark tests.

@antoninbas antoninbas merged commit a9e36d6 into vmware:main Oct 4, 2023
9 checks passed
@antoninbas antoninbas deleted the add-benchmarks branch October 4, 2023 03:29
heanlan pushed a commit to heanlan/go-ipfix that referenced this pull request Dec 7, 2023
A few new benchmarks, which can be useful when validating improvements
to the implementation.

Signed-off-by: Antonin Bas <[email protected]>
heanlan pushed a commit that referenced this pull request Dec 7, 2023
A few new benchmarks, which can be useful when validating improvements
to the implementation.

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.

3 participants