Improve IPFIX Set implementation #390
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Benchmark results without ResetSet improvements: And with the improvements: |
antoninbas
force-pushed
the
improve-set-implementation
branch
from
b7d06dd to
4f56128
Compare
yuntanghsu
reviewed
Dec 20, 2024
pkg/entities/set.go
Outdated
| @@ -45,6 +46,8 @@ const ( | |||
|
|
|||
| type Set interface { | |||
| PrepareSet(setType ContentType, templateID uint16) error | |||
| // Call ResetSet followd by a new call to PrepareSet in order to reuse an existing Set, | |||
There was a problem hiding this comment.
Suggested change
| // Call ResetSet followd by a new call to PrepareSet in order to reuse an existing Set, | |
| // Call ResetSet followed by a new call to PrepareSet in order to reuse an existing Set, |
| s.length = SetHeaderLen | ||
| } | ||
| s.setType = Undefined | ||
| s.records = nil | ||
| s.records = make([]Record, 0) | ||
| // Clear before shrinking the slice so that existing elements are eligible for garbage collection. |
There was a problem hiding this comment.
Do we need to reset the s.length here? I'm not quite sure what's the case when it isDecoding.
There was a problem hiding this comment.
I can reset it to 0, even though it's a change to the existing behavior. I agree that it would make more sense.
I am not a big fan of the isDecoding flag everywhere. We should have Marhsal / Unmarshal methods as appropriate, but that will have to be for a later time...
pkg/entities/set.go
Outdated
| // Sanity check: we need to make sure that the record is allowed to be added. | ||
| recordType := record.GetRecordType() | ||
| if recordType != s.setType { | ||
| return fmt.Errorf("Record and Set types don't match") |
There was a problem hiding this comment.
Suggested change
| return fmt.Errorf("Record and Set types don't match") | |
| return fmt.Errorf("record and Set types don't match") |
pkg/entities/set.go
Outdated
| return fmt.Errorf("Record and Set types don't match") | ||
| } | ||
| if recordType == Data && record.GetTemplateID() != s.templateID { | ||
| return fmt.Errorf("All Data Records in the same Data Set must have the same template ID") |
There was a problem hiding this comment.
Suggested change
| return fmt.Errorf("All Data Records in the same Data Set must have the same template ID") | |
| return fmt.Errorf("all Data Records in the same Data Set must have the same template ID") |
* Optimize ResetSet to avoid memory allocations * Add new interface method AddRecordV3 which takes a Record as a parameter (instead of a list of IEs). AddRecordV3 can avoid all memory allocations. We also add a new GetRecordType method to the Record interface. Signed-off-by: Antonin Bas <antonin.bas@broadcom.com>
Signed-off-by: Antonin Bas <antonin.bas@broadcom.com>
antoninbas
force-pushed
the
improve-set-implementation
branch
from
4f56128 to
e18a3d8
Compare
|
@yuntanghsu Thanks for the review, I addressed comments |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We also add a new GetRecordType method to the Record interface.