Optimize quickfixType Function for Performance & Readability #691
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
| allowedValues := d.FieldTypeByTag[int(field.tag)].Enums | ||
| if len(allowedValues) != 0 { | ||
| if _, validValue := allowedValues[string(field.value)]; !validValue { | ||
| return ValueIsIncorrect(field.tag) | ||
| } | ||
| } | ||
| // Define field type mappings for easy lookup | ||
| fieldTypeMapping := map[string]FieldValue{ |
There was a problem hiding this comment.
This instantiation performs a lot of unnecessary allocations which will negatively impact performance
There was a problem hiding this comment.
I do like the addition of comments on this path to help the developer, but on a hot path (versus the code generation path you changed otherwise) a switch statement will perform better than a map lookup. A blurb on why that’s the case: https://stackoverflow.com/questions/46789259/map-vs-switch-performance-in-go
| case "PERCENTAGE": | ||
| fallthrough | ||
| case "FLOAT": | ||
| // Define mappings of FIX field types to QuickFIX types |
There was a problem hiding this comment.
Now I do like the change here because it’s code generation and readability is more important than performance in this regard
|
Can you provide any benchmarks? |
# for free
to join this conversation on GitHub.
Already have an account?
# 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.
Key Optimizations:
✅ Replaces
fallthroughwith map lookups• Instead of a long switch with repeated
fallthroughstatements, we usemap[string]bool.• This makes lookups
O(1)instead of scanning all cases inO(n)time.✅ Improves Readability
• Grouping similar types into separate maps makes the function easier to read and maintain.
• The switch statement is shorter and clearer.
✅ Eliminates Unnecessary Variables
• Returns values directly instead of assigning to
quickfixType.• Instead of assigning prototype inside a switch, we fetch it directly from the map.
• If the type doesn’t exist in
fieldTypeMapping, the function exits early.