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

Leverage modern .NET features for improved perf. #803

Merged
merged 1 commit into from
Dec 8, 2022
Merged

Leverage modern .NET features for improved perf. #803

merged 1 commit into from
Dec 8, 2022

Conversation

geeknoid
Copy link
Contributor

@geeknoid geeknoid commented Dec 5, 2022

Changes

  • When serializing objects, recognize ISpanFormattable on .NET 6 in order to avoid a temp string allocation in that case.

  • Use binary primitives to serialize scalar types more efficiently (avoiding repeated bound checks).

@geeknoid geeknoid requested a review from a team December 5, 2022 01:49
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Dec 5, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: geeknoid / name: Martin Taillefer (9fc10e2)

@geeknoid
Copy link
Contributor Author

geeknoid commented Dec 5, 2022

I signed the CLA, but it's still showing as missing. Maybe there's latency there, or maybe something is messed up...

@Kielek
Copy link
Contributor

Kielek commented Dec 5, 2022

@geeknoid, if we speaking about CLA - please check if your email assigned to the commit is the same as you have assigned to you GitHub account. It is the most common issue I have seen here.

Copy link
Member

@CodeBlanch CodeBlanch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM thanks @geeknoid

@github-actions github-actions bot requested a review from CodeBlanch December 5, 2022 19:45
@geeknoid
Copy link
Contributor Author

geeknoid commented Dec 5, 2022

I've filed a support ticket with the EasyCLA folks regarding the fact even though I accepted the CLA, their bot is not happy about it. Hopefully, that'll be resolved soon.

@utpilla utpilla added the comp:exporter.geneva Things related to OpenTelemetry.Exporter.Geneva label Dec 5, 2022
Copy link

@teo-tsirpanis teo-tsirpanis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can share more code between .NET Standard 2.0 and .NET 6 if you:

  • Use the System.Memory package in .NET Standard 2.0 (still multi-target to avoid the reference on .NET 6). It also brings BinaryPrimitives downlevel.
  • Add an extension method on Encoding.GetBytes that takes spans, whose implementation pins them and calls the pointer overload.

- When serializing objects, recognize ISpanFormattable on .NET 6
to avoid a temp string allocation in that case.

- Use binary primitives to serialize scalar types more efficiently,
avoiding repeated bound checks.
@geeknoid
Copy link
Contributor Author

geeknoid commented Dec 7, 2022

OK, CLA issue is resolved. Now I just need to get the coverage to pass :-)

@utpilla
Copy link
Contributor

utpilla commented Dec 8, 2022

OK, CLA issue is resolved. Now I just need to get the coverage to pass :-)

That's a flaky test and it's not a required check.

Thanks for your contribution @geeknoid !

@utpilla utpilla merged commit c49a261 into open-telemetry:main Dec 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:exporter.geneva Things related to OpenTelemetry.Exporter.Geneva
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants