-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[chore] [pdata] Avoid an allocation when copy an optional field #13290
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
[chore] [pdata] Avoid an allocation when copy an optional field #13290
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #13290 +/- ##
==========================================
- Coverage 91.64% 91.63% -0.02%
==========================================
Files 522 522
Lines 29183 29223 +40
==========================================
+ Hits 26746 26778 +32
- Misses 1920 1926 +6
- Partials 517 519 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
contrib tests are fixed in open-telemetry/opentelemetry-collector-contrib#40978 |
ebfd397
to
46f8c45
Compare
dest{{ .fieldName }} = &{{ .originStructType }}{} | ||
dest.{{ .fieldName }}_ = dest{{ .fieldName }} | ||
} | ||
dest{{ .fieldName }}.{{ .fieldName }} = src.Get{{ .fieldName }}() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why using the Getter that checks for nil since is not nil?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. Updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bogdandrutu please take another look since it required significant enough change
|
||
const optionalPrimitiveCopyOrigTemplate = `if src.{{ .fieldName }}_ == nil { | ||
dest.{{ .fieldName }}_ = nil | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Can we return instead here, avoids an indentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it's rendered as part of copying one field while there may be others after that. Only if we move this to a separate function
46f8c45
to
3090436
Compare
Addresses #13267 (comment)