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 handling of string IEs #322

Merged
merged 2 commits into from
Sep 29, 2023

Conversation

antoninbas
Copy link
Member

  • Use the copy built-in to copy string bytes to buffer, instead of copying bytes one-by-one with a for loop. This is more efficient (confirmed with new Benchmark functions) and more correct (in case the string includes UTF-8 characters which use more than 1 byte).
  • Increase max string length from 65,534 to 65,535.

@antoninbas
Copy link
Member Author

New benchmark results:

BenchmarkEncodeInfoElementValueToBuffShortString-12    	    2770	    435264 ns/op	       0 B/op	       0 allocs/op
BenchmarkEncodeInfoElementValueToBuffLongString-12     	    1341	    824074 ns/op	       0 B/op	       0 allocs/op

Without the use of copy:

BenchmarkEncodeInfoElementValueToBuffShortString-12    	    2154	    531415 ns/op	       0 B/op	       0 allocs/op
BenchmarkEncodeInfoElementValueToBuffLongString-12     	     152	   7948069 ns/op	       0 B/op	       0 allocs/op

* Use the copy built-in to copy string bytes to buffer, instead of
  copying bytes one-by-one with a for loop. This is more efficient
  (confirmed with new Benchmark functions) and more correct (in case
  the string includes UTF-8 characters which use more than 1 byte).
* Increase max string length from 65,534 to 65,535.

Signed-off-by: Antonin Bas <abas@vmware.com>
@codecov
Copy link

codecov bot commented Sep 29, 2023

Codecov Report

Merging #322 (81d6724) into main (d0294af) will decrease coverage by 0.11%.
The diff coverage is 18.18%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #322      +/-   ##
==========================================
- Coverage   73.54%   73.44%   -0.11%     
==========================================
  Files          18       18              
  Lines        2786     2790       +4     
==========================================
  Hits         2049     2049              
- Misses        572      574       +2     
- Partials      165      167       +2     
Flag Coverage Δ
integration-tests 51.38% <ø> (ø)
unit-tests 72.43% <18.18%> (-0.11%) ⬇️

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

Files Coverage Δ
pkg/entities/ie.go 52.39% <18.18%> (-0.54%) ⬇️

dreamtalen
dreamtalen previously approved these changes Sep 29, 2023
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

pkg/entities/ie_test.go Outdated Show resolved Hide resolved
Signed-off-by: Antonin Bas <abas@vmware.com>
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

@antoninbas antoninbas merged commit b8729db into vmware:main Sep 29, 2023
7 of 9 checks passed
@antoninbas antoninbas deleted the improve-handling-of-strings branch September 29, 2023 18:02
dreamtalen pushed a commit to dreamtalen/go-ipfix that referenced this pull request Oct 2, 2023
* Use the copy built-in to copy string bytes to buffer, instead of
  copying bytes one-by-one with a for loop. This is more efficient
  (confirmed with new Benchmark functions) and more correct (in case
  the string includes UTF-8 characters which use more than 1 byte).
* Increase max string length from 65,534 to 65,535 (as per the
  IPFIX RFC 7011).

Signed-off-by: Antonin Bas <abas@vmware.com>
dreamtalen pushed a commit that referenced this pull request Oct 2, 2023
* Use the copy built-in to copy string bytes to buffer, instead of
  copying bytes one-by-one with a for loop. This is more efficient
  (confirmed with new Benchmark functions) and more correct (in case
  the string includes UTF-8 characters which use more than 1 byte).
* Increase max string length from 65,534 to 65,535 (as per the
  IPFIX RFC 7011).

Signed-off-by: Antonin Bas <abas@vmware.com>
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